Differential D101142
[SimplifyCFG/JumpThreading] Do not simplify empty blocks with unconditional branches if this causes loop metadata confusion. Authored by Meinersbur on Apr 23 2021, 12:14 AM.
Details When simplifying an empty block with an unconditional branch by skipping over it, the skipped block might have been a loop's latch. In that case transfer the loop's metadata attached to the latch to the new latch. If it already is also a latch with different metadata, do not skip the block. Part of fix for llvm.org/PR50060.
Diff Detail
Event TimelineComment Actions I'm not really in favor of using LoopHeaders for this. Comment Actions LoopHeaders is already used by both SimplifyCFG and JumpThreading. If it is stale and incomplete, it is already a problem now. Do you have an alternative suggestion? Comment Actions Did you verify the effect of this on the performance ? In a similar (but different) trial where I tried to block merging of blocks when that would result in a conflict with loop annotations, I noticed unacceptable regressions. Comment Actions Patch's subject is wrong. This doesn't "Transfer loop metadata to new latch.", it prevents folding. I don't know, but not introducing more problems by using problematic interface (LoopHeaders) seems like a good suggestion. Comment Actions I did not. This is a correctness fix. Arguing for performance over correctness feels weird. Fixed title.
To identify which blocks are latches. If moving metadata from another loop there may cause correctness problems, for instance if a mustprogress annotation is associated with a loop that does not need to progress. Unfortunately the metadata itself is not sufficient. Let A and B be to blocks which are latches of two different loops. A has metadata, but B has not. Combining the blocks and using the metadata of A for it will be wrong, as it would apply the same metadata (e.g. mustprogress) to both loops, as happened in the PR50060. The alternative would be to not combine any blocks if any of the two has loop metadata. Or dropping both metadata. Comment Actions What i meant is, doesn't that metadata belong to the loop latches? Comment Actions It indeed feels weird and maybe your testing shows that it is not that big of a problem. My feeling was that maybe a different approach in describing loop information might be more appropriate, resulting in accurate information, without the observed performance impact. That given, I have no idea yet how that could look like :(. Comment Actions This is what my example was trying to show. A latch that previously had no metadata must not be assigned metadata from a different loop. This is not about preserving loop metadata, but ensuring that the loop does not get assigned the wrong metadata. Like a mustprogress annotation or a #pragma clang loop vectorize(assume_safety) from a different loop. I agree that a different approach might be better, such as Johannes' suggestion to allow Metadata on (Header) BasicBlocks (which may come with other problems), or introducing an intrinsic that holds that metadata (which comes with a performance penalty of a different kind: every other pass has to iterate over ir). But until this happens, this fixes a potential miscompilation.
| ||||||||||||||||||||||||||||||