In response to the review: MAJOR — GpuMemoryUtilization not added to KVCacheConfig (R16) - this is the exact change I overwrote, adding this to the SimConfig would require massive changes everywhere the simulator/KVCacheConfig is initialized. I can edit the PR description to reflect what is actually implemented - but the review does have 1 valid point - gpu-memory-utilization stays an ad-hoc arg that is not reported back to the user when the sim results are run.

How do we want to handle this?

Response + complete fix list

Thanks for the honest breakdown. Your instinct is right — the current design (passing gpuMemoryUtilization as a function argument to CalculateKVBlocks) is architecturally correct and there is no R16 violation.

Here is the reasoning: KVCacheConfig holds simulation-time parameters. gpuMemoryUtilization is a pre-computation input that produces totalKVBlocks, which is then stored in KVCacheConfig. The simulation engine itself never needs the utilization fraction again. Putting a dead field in KVCacheConfig just to satisfy R16 would be the wrong trade-off. The "massive changes" concern is also not accurate — there are only 2 production NewKVCacheConfig call sites — but it doesn't matter, because the change isn't needed.

The real gap you identified is: the used value is invisible to the user. That is fixed with one logrus.Infof line (see Fix 3 below), not a struct change.

What must be fixed before this can merge

There are 6 concrete changes. Each one is specified exactly — file, location, old code, new code.

Fix 1 — Remove the committed binary

git rm inference-sim
echo "inference-sim" >> .gitignore
git add .gitignore

The compiled binary inference-sim was accidentally committed. It must not be in the repository.

Fix 2 — Use the flag value in cmd/replay.go (the critical bug)

File: cmd/replay.go

Change A — line 377: Replace the hardcoded 0.9 with the variable.

// BEFORE
autoBlocks, calcErr := latency.CalculateKVBlocks(modelConfig, hwConfig, tensorParallelism, blockSizeTokens, 0.9, kvParams)

// AFTER
autoBlocks, calcErr := latency.CalculateKVBlocks(modelConfig, hwConfig, tensorParallelism, blockSizeTokens, gpuMemoryUtilization, kvParams)

Change B — after line 533 (the kv-offload-threshold validation block): add the same validation guard that runCmd has.

// Add this block immediately after the existing kv-offload-threshold guard:
if gpuMemoryUtilization <= 0 || gpuMemoryUtilization > 1.0 || math.IsNaN(gpuMemoryUtilization) || math.IsInf(gpuMemoryUtilization, 0) {
    logrus.Fatalf("--gpu-memory-utilization must be a finite value in (0, 1.0], got %f", gpuMemoryUtilization)
}

Fix 3 — Log the utilization value when auto-calculation succeeds

The flag value should appear in stderr so users can see what was used. Add one logrus.Infof line inside the else block where totalKVBlocks = autoBlocks is assigned — in both runCmd and replayCmd.

In cmd/root.go the existing block looks like:

} else {
    totalKVBlocks = autoBlocks
    logrus.Infof("--latency-model blackbox: auto-calculated total-kv-blocks=%d ...", totalKVBlocks)
}

Add immediately after totalKVBlocks = autoBlocks (or as part of the existing logrus.Infof):

logrus.Infof("--gpu-memory-utilization: %.2f used for KV block auto-calculation", gpuMemoryUtilization)

Do the same in cmd/replay.go after the matching totalKVBlocks = autoBlocks line (also inside the CalculateKVBlocks success branch).

Fix 4 — Decide the validation range and make all three places agree

Currently the code accepts (0, 1.0] but the PR description says [0.5, 1.0]. Pick (0, 1.0] — it is more general and the code is already correct.

Update three places to match:

1. cmd/root.go flag description (line 267): change "Fraction of GPU memory to use for KV cache (>0 to 1.0). Default: 0.9 (90%)" → "Fraction of GPU memory to use for KV cache, in the range (0, 1.0]. Default: 0.9 (90%)"

2. PR description BC-1: change "KVCacheConfig accepts GpuMemoryUtilization with validation [0.5, 1.0]" → "CalculateKVBlocks validates gpuMemoryUtilization in range (0, 1.0]"

3. The test name in TestCalculateKVBlocks_GpuMemoryUtilization_Validation: rename "below range" to "zero is invalid" and "above range" to "above 1.0 is invalid" so they describe the actual boundary being tested.

Fix 5 — Remove the unrelated TFlopsFP8 hunks

The changes to sim/latency/config.go and sim/latency/config_test.go in this PR belong to PR #765, not here. Remove them to keep this PR atomic. Specifically, remove:

• In sim/latency/config.go: the 3-line block adding TFlopsFP8 validation in ValidateRooflineConfig
• In sim/latency/config_test.go: the three new test functions TestValidateRooflineConfig_TFlopsFP8_NegativeReturnsError, TestValidateRooflineConfig_TFlopsFP8_NaNReturnsError, and TestValidateRooflineConfig_TFlopsFP8_ZeroIsValid

If #765 is already merged, these will cause a merge conflict anyway. If it hasn't merged yet, they still don't belong here.

Fix 6 — Add the missing invariant test (R7)

The existing TestCalculateKVBlocks_GpuMemoryUtilization_Validation only tests error conditions. R7 requires an invariant test verifying the behavioral law. Add this function to sim/latency/kv_capacity_test.go:

func TestCalculateKVBlocks_GpuMemoryUtilization_HigherUtilProducesMoreBlocks(t *testing.T) {
    // BC: higher GPU memory utilization leaves more memory for KV cache → more blocks
    // GIVEN the same model and hardware
    mc := validDenseModelConfig()
    hc := validHWConfig()
    params := validDenseKVParams()

    // WHEN computed at 90% vs 95% utilization
    blocks90, err := latency.CalculateKVBlocks(mc, hc, 1, 16, 0.90, params)
    if err != nil {
        t.Fatalf("util=0.90: unexpected error: %v", err)
    }
    blocks95, err := latency.CalculateKVBlocks(mc, hc, 1, 16, 0.95, params)
    if err != nil {
        t.Fatalf("util=0.95: unexpected error: %v", err)
    }

    // THEN higher utilization must produce strictly more blocks
    if blocks95 <= blocks90 {
        t.Errorf("util=0.95 should yield more blocks than util=0.90: got %d vs %d", blocks95, blocks90)
    }
}

Fix 7 — Add missing parameter to CalculateKVBlocks doc comment

File: sim/latency/kv_capacity.go, in the doc comment directly above func CalculateKVBlocks(...).

The current Parameters: list ends at params. Add one line:

//   - gpuMemoryUtilization: fraction of GPU HBM available for KV cache, in (0, 1.0]

Fix 8 — Update the PR description to match reality

Replace the four false behavioral contract claims with accurate ones:

## Behavioral Contracts

- BC-1: CalculateKVBlocks validates gpuMemoryUtilization in (0, 1.0]; returns error on zero, negative, >1.0, NaN, Inf
- BC-2: Default 0.9 (via CLI flag default) preserves existing behavior for all callers that do not pass the flag
- BC-3: CalculateKVBlocks uses the configurable parameter instead of the hardcoded constant
- BC-4: --gpu-memory-utilization flag is registered for both `blis run` and `blis replay`; the value is validated and logged when auto-calculation succeeds

Also remove the phrases "pointer type per R9", "nil defaults to 0.9", and "92 construction sites, 38 CalculateKVBlocks calls" — none of these are true of the actual implementation.

Checklist

• inference-sim binary removed from git, added to .gitignore
• cmd/replay.go:377 uses gpuMemoryUtilization (not 0.9)
• cmd/replay.go has the logrus.Fatalf validation guard for gpuMemoryUtilization
• Both runCmd and replayCmd log the utilization value when auto-calculation succeeds
• Validation range is (0, 1.0] consistently in the guard, error message, flag description, and test names
• sim/latency/config.go and sim/latency/config_test.go TFlopsFP8 hunks removed
• TestCalculateKVBlocks_GpuMemoryUtilization_HigherUtilProducesMoreBlocks added
• CalculateKVBlocks doc comment lists gpuMemoryUtilization parameter
• PR description BCs updated to match the actual implementation
• go test ./... passes