Page MenuHomePhabricator

[BasicBlockUtils] Add metadata fixing in SplitBlockPredecessors.
Needs ReviewPublic

Authored by aus_intel on Aug 28 2019, 9:43 AM.

Details

Summary

In case when BB is header of some loop and predecessor is latch of
this loop, metadata was not attached to newly created basic block.
This led to loss of loop metadata for other passes.

Diff Detail

Event Timeline

aus_intel created this revision.Aug 28 2019, 9:43 AM
jdoerfert edited reviewers, added: Meinersbur; removed: llvm-commits.Aug 28 2019, 11:01 AM
jdoerfert added a subscriber: llvm-commits.

I don't feel confident enough in this area to give an official LGTM but If I understand it correctly the patch seems reasonable: move loop metadata from old to new latch when a loop header is split.

I have left some inline comments in the test.

Thanks,
Orlando

llvm/test/Transforms/LoopSimplify/loop_metadata.ll
1

Please could you add a short description describing what the test is for (it's not obvious just looking at the test file)?

It's not a requirement but it is helpful for others if this test ever fails.

4

I think this should be a CHECK-NEXT to ensure this branch is part of the for.cond.loopexit BB.

5

I assume this line is to check that the loop metadata is correctly removed from this block.

However, the CHECK will still pass with:

br i1 %cmp1, label %for.body1, label %for.cond.loopexit, !llvm.loop !0

I think you want the following instead, because {{$}} will match the end of the line.

CHECK: br i1 %cmp1, label %for.body1, label %for.cond.loopexit{{$}}
Meinersbur added inline comments.Sep 25 2019, 2:43 PM
llvm/lib/Transforms/Utils/BasicBlockUtils.cpp
582

Note that is is only the case if Pred contains all predecessors from outside the loop. If there is an edge to BB left, the new block does not match the definition of a preheader. If Pred contains a backedge, the new block will be inside the loop.

602

Could you be more specific in the comment, such as:

// Move the loop metadata from the old latch terminator to the new basic block, which replaces BB as the latch.
605

BB may continue to be a latch if it has multiple edges to the header, typical for switch terminators.

However, I am not sure it is worth adding a test. We could just leave the LoopMD at the old terminator as well since without being a latch, it should never be used.

(Could you add a test for this case, independent of what we decide to do?)