This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG/JumpThreading] Do not simplify empty blocks with unconditional branches if this causes loop metadata confusion.
Needs ReviewPublic

Authored by Meinersbur on Apr 23 2021, 12:14 AM.

Details

Summary

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 Timeline

Meinersbur created this revision.Apr 23 2021, 12:14 AM
Meinersbur requested review of this revision.Apr 23 2021, 12:14 AM
lebedev.ri added a subscriber: lebedev.ri.

I'm not really in favor of using LoopHeaders for this.
It is not updated *anywhere*, which means that most of the time it's data is both stale and incomplete.

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?

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.

Patch's subject is wrong. This doesn't "Transfer loop metadata to new latch.", it prevents folding.
Or is this a wrong diff?

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?

I don't know, but not introducing more problems by using problematic interface (LoopHeaders) seems like a good suggestion.
if you already have metadata, why do you need LoopHeaders ?
But basically, i'm unconvinced that we want to restrict simplifycfg in this way.

Meinersbur retitled this revision from [SimplifyCFG/JumpThreading] Transfer loop metadata to new latch. to [SimplifyCFG/JumpThreading] Do not simplify empty blocks with unconditional branches if this causes loop metadata confusion..Apr 23 2021, 12:57 AM

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.

I did not. This is a correctness fix. Arguing for performance over correctness feels weird.

Patch's subject is wrong. This doesn't "Transfer loop metadata to new latch.", it prevents folding.

Fixed title.

if you already have metadata, why do you need LoopHeaders ?

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.

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.

I did not. This is a correctness fix. Arguing for performance over correctness feels weird.

Patch's subject is wrong. This doesn't "Transfer loop metadata to new latch.", it prevents folding.

Fixed title.

if you already have metadata, why do you need LoopHeaders ?

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.

What i meant is, doesn't that metadata belong to the loop latches?
So if we really want to treat loop latches specially, isn't merely the presence of said metadata enough?

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.

I did not. This is a correctness fix. Arguing for performance over correctness feels weird.

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 :(.

What i meant is, doesn't that metadata belong to the loop latches?
So if we really want to treat loop latches specially, isn't merely the presence of said metadata enough?

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.

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 :(.

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.

llvm/lib/Transforms/Utils/Local.cpp
1090

Is (LoopMD != PredMD) a sufficient rule ?

Assume you have two nested loops with both a 'loop unroll 4' annotation. Can we get in a situation where both latches would be merged ? If so, is it valid then to still merge the latches ?

Meinersbur added inline comments.May 18 2021, 12:34 PM
llvm/lib/Transforms/Utils/Local.cpp
1090

The idea was that if the loops have the same properties, then they will still have the same properties after merging. That itself might be problematic since changing the LoopMD (e.g. LoopVectorize sets a isvectorized property when unable to vectorize) would apply to both loops.

However, also conside LoopMD are distinct and therefore different loop should have different MDNodes. This is not strictly enforced (e.g. Inliner or LoopUnroll duplicate entire loops), so it can happen for different loops to have the same LoopMD.

I can make the condition stricter by comparing the header blocks they branch to.

Is there still interest in this patch?

lebedev.ri resigned from this revision.Jan 12 2023, 5:21 PM

This review seems to be stuck/dead, consider abandoning if no longer relevant.

Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 5:21 PM