Mark wave intrinsics as convergent to prevent loop miscompilation#8204
Mark wave intrinsics as convergent to prevent loop miscompilation#8204JoeCitizen wants to merge 3 commits intomicrosoft:mainfrom
Conversation
The JumpThreading pass could restructure loops containing wave intrinsics by threading edges through the loop latch block. This moved wave ops (WaveReadLaneFirst, WaveActiveCountBits, etc.) from inside the loop to after it, changing the set of active lanes at the call site. On SIMT hardware, this produced incorrect results because all lanes reconverge at the post-loop point rather than only the matching subset. Fix: - DxilConvergentMark: Mark wave-sensitive HL functions with the Attribute::Convergent attribute before optimizer passes run. - JumpThreading: In ThreadEdge, walk backward from the latch to the loop header to identify loop body blocks. If any contains a convergent call, prevent threading through the latch. - DxilOperations::GetOpFunc: Mark DXIL wave op functions with Attribute::Convergent for post-dxilgen optimizer passes. Fixes a bug where a material binning pattern using WaveReadLaneFirst + WaveActiveCountBits in a while-loop produced correct results at -Od but incorrect results with optimizations enabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
lib/DXIL/DxilOperations.cpp
Outdated
| if (existF->getFunctionType() != pFT) | ||
| return nullptr; | ||
| F = existF; | ||
| // HLSL Change Begin - ensure attributes are set on existing functions. |
There was a problem hiding this comment.
These HLSL Change markers are also not needed.
- Rename bUpdated -> Updated (drop Hungarian prefix per reviewer feedback) - Remove // HLSL Change Begin/End markers in DxilConvergent.cpp since the file is already HLSL-specific Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
JoeCitizen
force-pushed
the
user/jackell/prefix-sum-bug
branch
from
fa67aa2 to
8dc2b90
Compare
tex3d
left a comment
tex3d
left a comment
There was a problem hiding this comment.
I have several problems with this change in its current form.
- Change impacts final DXIL
- Test JumpThreading pass change with a pass test under DXC/Passes
- Let's not add tests under HLSLFileCheck if we can avoid it.
- It looks like JumpThreading is run before DxilGeneration, so how could convergent attr on the DXIL op make a difference during this pass when you only have the HL ops during this pass?
| F->addFnAttr(OpProps.FuncAttr); | ||
| // Mark wave ops as convergent since they depend on the active lane set. | ||
| if (IsDxilOpWave(opCode) && !F->hasFnAttribute(Attribute::Convergent)) | ||
| F->addFnAttr(Attribute::Convergent); |
There was a problem hiding this comment.
This change modifies attributes that show up in final DXIL. We should be cautious with changes like that, since it could break drivers not expecting it. We need to decide whether we want to expose the convergent attribute in next DXIL version and probably filter attributes on DXIL ops for prior shader models in DxilFinalizeModule (defined in DxilPreparePasses.cpp).
There was a problem hiding this comment.
We need a pass test (under DXC/Passes) if you're testing the JumpThreading pass. Plus this test is under the older TAEF test suite, and isn't run as a lit shell test. It would be better to keep additions under the lit shell test locations. For a test compiling HLSL to DXIL, I think that would be under CodeGenDXIL.
3 participants
The JumpThreading pass could restructure loops containing wave intrinsics by threading edges through the loop latch block. This moved wave ops (WaveReadLaneFirst, WaveActiveCountBits, etc.) from inside the loop to after it, changing the set of active lanes at the call site. On SIMT hardware, this produced incorrect results because all lanes reconverge at the post-loop point rather than only the matching subset.
Fix:
Fixes a bug where a material binning pattern using WaveReadLaneFirst + WaveActiveCountBits in a while-loop produced correct results at -Od but incorrect results with optimizations enabled.