Repository navigation
Make DxilValueCache a FunctionPass analysis - #9094
Open
Steven Perron (s-perron) wants to merge 4 commits into
Open
Steven Perron (s-perron) wants to merge 4 commits into
Steven Perron (s-perron) wants to merge 4 commits into
Conversation
As an ImmutablePass, DxilValueCache was never invalidated. This is not
the intended use of ImmutablePass, since the DxilValueCache analysis can
change as the code changes. The motivating example is Reassociate
rewriting an expression in place:
```
%b = %a + 1 // %a is provably a constant, so %b is as well.
%c = %d + %b
==>
%b = %d + 1 // %b is no longer a constant, but DxilValueCache still
// thinks it is.
%c = %b + %a
```
The stale data in DxilValueCache can lead other transformations to make
mistakes; here DxilRemoveDeadBlocks deleted a live loop exit.
The solution is to change DxilValueCache from an ImmutablePass to a
FunctionPass, modeled after LazyValueInfo and its LazyValueInfoCache.
That way the pass manager invalidates it after any pass that does not
claim to preserve it. Module passes that use it now query it per
function.
ScalarEvolution is modified to use a local DxilValueCache instead. The
loop passes preserve ScalarEvolution but not DxilValueCache, and the
legacy pass manager does not invalidate an analysis when one it depends
on is invalidated. Because ScalarEvolution queries the cache lazily,
after those passes have run, a shared instance could still hold values
computed before they rewrote the function. A cache that lives only for
one exhaustive trip count evaluation avoids that.
Steven Perron (s-perron)
requested review from
Chris B (llvm-beanz) and
Natalie Chouinard (sudonatalie)
November 3, 2026 18:36
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Repeated DxilValueCache requests clear reusable analysis and can cause quadratic work, while the regression does not independently exercise pass-managed cache invalidation.
3 open issues
What changed in this PR
This PR makes DxilValueCache a function-level analysis so transformations invalidate stale results, addressing a miscompile that could remove live loop exits.
Changes:
- Add cache reset hooks and update module-pass consumers to request analysis per function.
- Give ScalarEvolution a private cache for each exhaustive trip-count evaluation.
- Add a regression test for stale values after reassociation.
| File | Description |
|---|---|
tools/clang/test/DXC/Passes/DxilRemoveDeadBlocks/stale-value-cache.ll |
Tests preservation of live loop exits after reassociation. |
lib/HLSL/DxilNoOptLegalize.cpp |
Requests the cache per nonempty function. |
lib/HLSL/DxilCondenseResources.cpp |
Requests function-scoped analysis during resource legalization. |
lib/Analysis/ScalarEvolution.cpp |
Uses a private cache for exhaustive trip-count evaluation. |
lib/Analysis/DxilValueCache.cpp |
Implements function-pass initialization and cache reset behavior. |
include/llvm/Analysis/DxilValueCache.h |
Changes the base class and declares lifecycle hooks. |
Review effort: Balanced
Comment on lines
+478
to
+480
| bool DxilValueCache::runOnFunction(Function &F) { | ||
| // Values are computed lazily by GetValue, so just drop the previous entries. | ||
| ResetAll(); |
Collaborator
Author
There was a problem hiding this comment.
Chris B (@llvm-beanz) Let me know if I need one.
…ectly The printer computes the constant value of every instruction, so a test can build the cache, run a pass that rewrites an instruction in place, and check that the stale value is not reported afterwards.
Every getAnalysis(F) call from a module pass reruns the on-the-fly pass manager and empties the cache, so legalize each function's resource arrays and PHIs with a single query instead of one per alloca and per PHI.
Printing no longer computes values, so the test seeds the cache through dxil-elim-vector before reassociate rewrites %X in place.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As an ImmutablePass, DxilValueCache was never invalidated. This is not
the intended use of ImmutablePass, since the DxilValueCache analysis can
change as the code changes. The motivating example is Reassociate
rewriting an expression in place:
The stale data in DxilValueCache can lead other transformations to make
mistakes; here DxilRemoveDeadBlocks deleted a live loop exit.
The solution is to change DxilValueCache from an ImmutablePass to a
FunctionPass, modeled after LazyValueInfo and its LazyValueInfoCache.
That way the pass manager invalidates it after any pass that does not
claim to preserve it. Module passes that use it now query it per
function.
ScalarEvolution is modified to use a local DxilValueCache instead. The
loop passes preserve ScalarEvolution but not DxilValueCache, and the
legacy pass manager does not invalidate an analysis when one it depends
on is invalidated. Because ScalarEvolution queries the cache lazily,
after those passes have run, a shared instance could still hold values
computed before they rewrote the function. A cache that lives only for
one exhaustive trip count evaluation avoids that.