Repository navigation
Fix double-increment bug in LegalizeResources when looping over PHIs - #7934
Tex Riddell (tex3d) wants to merge 2 commits into
Conversation
This subtle bug can cause a failure to resolve a static resource in a corner case where DxilValueCache could replace an odd-indexed phi, and this is needed to resolve other phis to known values, but an indexing bug prevents the loop from ever visiting any phi nodes at odd-numbered indexes in the list. This change also adds two IR-based pass tests for this code path, as there weren't any before.
|
Tex Riddell (@tex3d) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
There was a problem hiding this comment.
🟢 Approval recommended
The localized traversal fix handles erasure and termination correctly, with targeted regression coverage and no identified blocking issues.
No open issues 🎉
What changed in this PR
Fixes resource legalization skipping PHI nodes at odd-numbered indexes, allowing dependent PHIs to resolve to known resources.
Changes:
- Replaces index-based traversal with an iterator loop that safely handles erasure.
- Repeats simplification until all PHIs are resolved or no progress is made.
- Adds two IR regression tests covering resource resolution, including the dependent-PHI corner case.
| File | Description |
|---|---|
| tools/clang/test/DXC/Passes/DxilLowerCreateHandleForLib/legalize-resource-second-phi-first.ll | Tests resolution that depends on visiting the second PHI first. |
| tools/clang/test/DXC/Passes/DxilLowerCreateHandleForLib/legalize-resource-phi.ll | Tests basic PHI resource resolution and resource metadata cleanup. |
| lib/HLSL/DxilCondenseResources.cpp | Fixes PHI traversal and preserves progress-based termination. |
Review effort: Balanced
Deric C. (Icohedron)
left a comment
There was a problem hiding this comment.
The algorithm looks correct to me.
The pseudocode for the core part of this change seems to be:
-for (Attempt = 0; Attempt < PHIs.size(); Attempt++)
- for (i = 0; i < PHIs.size(); i++)
- if GetValue(PHIs[i]): RAUW, erase(i) // header i++ skips the shifted PHI
- else: i++ // header i++ skips the next PHI
- if no change: break // odd slots never tried
+while PHIs not empty
+ for each It in PHIs
+ if GetValue(*It): RAUW, It = erase(It)
+ else: ++It
+ if no change: break // every pass visits every PHIOne additional review comment I have is that the PR description states:
... an indexing bug prevents the loop from ever visiting any phi nodes at odd-numbered indexes in the list.
but that isn't precisely correct. It should instead read something like:
If no phi node at an even position resolves in a pass/attempt, the loop exits without trying phi nodes at odd positions.
| PHIs.erase(PHIs.begin() + i); | ||
| } else { | ||
| i++; | ||
| for (auto It = PHIs.begin(); It != PHIs.end();) { |
There was a problem hiding this comment.
nit: There should be a comment stating that this loop intentionally re-evaluates .end() every iteration.
LLVM Coding Standards say to not evaluate .end() every iteration of the loop. But in this case it is necessary because the container is being mutated, and therefore a comment is required to state that it is intentional.
https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop
There was a problem hiding this comment.
Though the coding standards does also say to prefer range-based for-loops I do agree this is easier to follow using an iterator-based loop.
| %cmp_ev = icmp eq i32 %ev, 0 | ||
| br i1 %cmp_ev, label %case1, label %loopback ; unknown branch | ||
|
|
||
| case1: ; preds = loopback, %entry |
There was a problem hiding this comment.
typo
| case1: ; preds = loopback, %entry | |
| case1: ; preds = %loopback, %entry |
This subtle bug can cause a failure to resolve a static resource in a corner case where DxilValueCache could replace an odd-indexed phi, and this is needed to resolve other phis to known values, but an indexing bug prevents the loop from ever visiting any phi nodes at odd-numbered indexes in the list.
This change also adds two IR-based pass tests for this code path, as there weren't any before.