[ScopeNestedCFG] Prevent blocks containing convergent instructions from being cloned#8142
[ScopeNestedCFG] Prevent blocks containing convergent instructions from being cloned#8142bob80905 wants to merge 8 commits intomicrosoft:mainfrom
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
|
||
| # Link against LLVMDXIL so that ScopeNestedCFG can use DxilUtil functions | ||
| target_link_libraries(DxilConvPasses | ||
| PRIVATE LLVMDXIL | ||
| ) | ||
|
|
||
| target_include_directories(DxilConvPasses | ||
| PRIVATE | ||
| ${DXC_SOURCE_DIR}/lib/DXIL | ||
| ${DXC_SOURCE_DIR}/lib/IR | ||
| ) |
There was a problem hiding this comment.
Are any of these changes necessary?
I don't see any obvious new external functions being called, so I'm not sure why a lib is being added. I don't see any new headers being included, so I don't understand why we need to add two directories to the include path.
| for (Instruction &I : *BB) { | ||
| if (auto *CI = dyn_cast<CallInst>(&I)) { | ||
| Function *CF = CI->getCalledFunction(); | ||
| if (CF && CF->hasFnAttribute(Attribute::AttrKind::Convergent)) |
There was a problem hiding this comment.
This probably doesn't actually fix the issue because we currently strip convergent attributes off DXIL operations before emitting the final DXIL so we would also need a pass to restore the convergent attribute.
I'm not sure what utilities DXC has for identifying convergent DXIL ops in absence of the attribute.
|
FYI, I'm looking at a workaround in WARP so that this change wouldn't be needed. |
llvm-beanz
left a comment
llvm-beanz
left a comment
There was a problem hiding this comment.
The other tests for the ScopeNestedCFG pass use the ScopeNestedInfo pass to print out the scope nesting. We should probably also update that pass and use it in the tests.
| return true; | ||
| // Account for derivative ops as well: | ||
| // DerivCoarseX = 83, DerivCoarseY = 84, DerivFineX = 85, DerivFineY = 86 | ||
| if (83 <= op && op <= 86) |
There was a problem hiding this comment.
This generated pattern shouldn't be hand-written. You really want to include all gradient ops (derivative ops and ops that use derivatives). It turns out there's already a IsDxilOpGradient with the op set generated from hctdb.py. You can call that here instead.
tex3d
left a comment
tex3d
left a comment
There was a problem hiding this comment.
This looks like it should be two separate PRs:
- Prevent clone of blocks with convergent operations
- Allow and mark switch fallthrough
This pass is complex. My intuition says these kinds of changes are risky. I kind of doubt that the new tests represent sufficient coverage of potential cases impacted, nor do I think existing tests have sufficient coverage now, given these changes. I think it would be necessary to clone and adapt some of the existing tests to expand coverage of these changes.
It is also likely important to ensure that preventing cloning doesn't break reducibility assumptions - like reducibility analysis would pass, but that might require cloning of blocks that contain convergent operations.
I haven't yet thoroughly reviewed the changes to the pass, but do have a concern about the change to the BranchKind enum, and wanted to express other general concerns here.
| SwitchEnd, | ||
| SwitchNoEnd, | ||
| SwitchBreak, | ||
| SwitchFallthrough, |
There was a problem hiding this comment.
This changes the subsequent Loop* values.
Since BranchKind is serialized to metadata, so this represents a breaking change in the output. While you could argue that anyone using this pass could synchronize updates to their code when consuming the new version of this pass, it would be a lot safer, and less destabilizing, to add the new value to the end instead.
| ; CHECK: exit | ||
| ; CHECK: @Switch_Break | ||
| ; CHECK: @Switch_Case | ||
| ; CHECK: case1 |
There was a problem hiding this comment.
Am I misreading this, or is case1 cloned here? Isn't that supposed to be prevented for blocks containing convergent ops? If this is a printing issue from scopenestinfo, shouldn't that pass be updated to properly express the CFG for fallthrough?
There was a problem hiding this comment.
Or maybe this is a block that wasn't cloned, but is being iterated twice by scope nested iterator? I'm not familiar enough with these and haven't loaded enough context to be sure. Either way, CHECK: case1 seems insufficient to differentiate case1 from something like case1.1 which could be produced by cloning.
|
See #8141 (comment) |
5 participants
This PR modifies the ScopeNestedCFG pass to prevent blocks containing convergent instructions from being cloned.
Previously, blocks would be cloned by
TransformAcyclicRegionand similar functions. There should be an exception when blocks contain wave ops.Fixes #8141