Skip to content

Commit afd5cc3

Browse files
IgWod-IMGIanWood1
authored andcommitted
[mlir][spirv] Update mergeInfo of blocks nested in regions (llvm#137789)
The current code that updates mergeInfo iterates only over constructBlocks, potentially leaving blocks nested in already structured regions with invalid mergeInfo. This patch adds walk for each block to ensure all nested blocks are considered. It is not possible to add a unit test exercising this change as whether the problem occurs depends on the structuring order that is currently non-deterministic.
1 parent ed502f2 commit afd5cc3

File tree

1 file changed

+62
-24
lines changed

1 file changed

+62
-24
lines changed

mlir/lib/Target/SPIRV/Deserialization/Deserializer.cpp

Lines changed: 62 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -2104,37 +2104,75 @@ LogicalResult ControlFlowStructurizer::structurize() {
21042104
// selection/loop. If so, they will be recorded within blockMergeInfo.
21052105
// We need to update the pointers there to the newly remapped ones so we can
21062106
// continue structurizing them later.
2107+
//
2108+
// We need to walk each block as constructBlocks do not include blocks
2109+
// internal to ops already structured within those blocks. It is not
2110+
// fully clear to me why the mergeInfo of blocks (yet to be structured)
2111+
// inside already structured selections/loops get invalidated and needs
2112+
// updating, however the following example code can cause a crash (depending
2113+
// on the structuring order), when the most inner selection is being
2114+
// structured after the outer selection and loop have been already
2115+
// structured:
2116+
//
2117+
// spirv.mlir.for {
2118+
// // ...
2119+
// spirv.mlir.selection {
2120+
// // ..
2121+
// // A selection region that hasn't been yet structured!
2122+
// // ..
2123+
// }
2124+
// // ...
2125+
// }
2126+
//
2127+
// If the loop gets structured after the outer selection, but before the
2128+
// inner selection. Moving the already structured selection inside the loop
2129+
// will invalidate the mergeInfo of the region that is not yet structured.
2130+
// Just going over constructBlocks will not check and updated header blocks
2131+
// inside the already structured selection region. Walking block fixes that.
2132+
//
2133+
// TODO: If structuring was done in a fixed order starting with inner
2134+
// most constructs this most likely not be an issue and the whole code
2135+
// section could be removed. However, with the current non-deterministic
2136+
// order this is not possible.
2137+
//
21072138
// TODO: The asserts in the following assumes input SPIR-V blob forms
21082139
// correctly nested selection/loop constructs. We should relax this and
21092140
// support error cases better.
2110-
auto it = blockMergeInfo.find(block);
2111-
if (it != blockMergeInfo.end()) {
2112-
// Use the original location for nested selection/loop ops.
2113-
Location loc = it->second.loc;
2114-
2115-
Block *newHeader = mapper.lookupOrNull(block);
2116-
if (!newHeader)
2117-
return emitError(loc, "failed control flow structurization: nested "
2118-
"loop header block should be remapped!");
2119-
2120-
Block *newContinue = it->second.continueBlock;
2121-
if (newContinue) {
2122-
newContinue = mapper.lookupOrNull(newContinue);
2123-
if (!newContinue)
2141+
auto updateMergeInfo = [&](Block *block) -> WalkResult {
2142+
auto it = blockMergeInfo.find(block);
2143+
if (it != blockMergeInfo.end()) {
2144+
// Use the original location for nested selection/loop ops.
2145+
Location loc = it->second.loc;
2146+
2147+
Block *newHeader = mapper.lookupOrNull(block);
2148+
if (!newHeader)
21242149
return emitError(loc, "failed control flow structurization: nested "
2125-
"loop continue block should be remapped!");
2150+
"loop header block should be remapped!");
2151+
2152+
Block *newContinue = it->second.continueBlock;
2153+
if (newContinue) {
2154+
newContinue = mapper.lookupOrNull(newContinue);
2155+
if (!newContinue)
2156+
return emitError(loc, "failed control flow structurization: nested "
2157+
"loop continue block should be remapped!");
2158+
}
2159+
2160+
Block *newMerge = it->second.mergeBlock;
2161+
if (Block *mappedTo = mapper.lookupOrNull(newMerge))
2162+
newMerge = mappedTo;
2163+
2164+
// The iterator should be erased before adding a new entry into
2165+
// blockMergeInfo to avoid iterator invalidation.
2166+
blockMergeInfo.erase(it);
2167+
blockMergeInfo.try_emplace(newHeader, loc, it->second.control, newMerge,
2168+
newContinue);
21262169
}
21272170

2128-
Block *newMerge = it->second.mergeBlock;
2129-
if (Block *mappedTo = mapper.lookupOrNull(newMerge))
2130-
newMerge = mappedTo;
2171+
return WalkResult::advance();
2172+
};
21312173

2132-
// The iterator should be erased before adding a new entry into
2133-
// blockMergeInfo to avoid iterator invalidation.
2134-
blockMergeInfo.erase(it);
2135-
blockMergeInfo.try_emplace(newHeader, loc, it->second.control, newMerge,
2136-
newContinue);
2137-
}
2174+
if (block->walk(updateMergeInfo).wasInterrupted())
2175+
return failure();
21382176

21392177
// The structured selection/loop's entry block does not have arguments.
21402178
// If the function's header block is also part of the structured control

0 commit comments

Comments
 (0)