[BasicBlockUtils] Do not move loop metadata if outer loop header.

[hgreving]

Summary

Fixes a bug preventing moving the loop's metadata to an outer loop's header,
which happens if the loop's exit is also the header of an outer loop.

Adds a test for above.

Fixes #55416.

[hgreving]

Whoa another corner case.

[fhahn]

Thanks for the patch! Makes sense, I added some suggestions with respect to the test.

llvm/test/Transforms/LoopSimplify/update_latch_md2.ll

4

nit: this uses the legacy PM syntax, would be good to update this to use -passes=loop-simplify,

14

it would make the test a bit easier to read if the names would be a bit more descriptive, e.g. _bb7 -> loop.header, _bb6 -> loop.latch, _bb2 -> exit and so on.

Also, would it be possible to pre-commit the cleaned up test and then only include the diff caused by the patch?

[hgreving]

Rebased to https://reviews.llvm.org/D125696

[hgreving]

In D125574#3515105, @fhahn wrote:

Thanks for the patch! Makes sense, I added some suggestions with respect to the test.

Should be done.

[hgreving]

In D125574#3516111, @hgreving wrote:

In D125574#3515105, @fhahn wrote:

Thanks for the patch! Makes sense, I added some suggestions with respect to the test.

Should be done.

[hgreving]

This comment was removed by hgreving.

[hgreving]

Friendly ping on this review.

[fhahn]

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp

1162

So the interesting case is when the original latch is shared between multiple loops. At the moment, we move the metadata to the new latch of the outer loop. With this patch we keep it at the latch of the inneer loop, which won't be the latch of the outer loop any longer.

I am not sure if either option is more correct/better than the other.

Should we duplicate the metadata and add it to the new latches of both loops? This would be in line with the original IR, where the metadata applied to both loops.

llvm/test/Transforms/LoopSimplify/update_latch_md2.ll

27

Ah, so the the llvm.loop metadata here applies to 2 loops. `loop.latch is shared between 2 loops.

[hgreving]

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp

1162

You're right, I think we should copy the metadata. I will update the patch.

[Meinersbur]

LGTM. Please wait for @fhahn LGTM as well.

[fhahn]

In D125574#3532385, @Meinersbur wrote:

LGTM. Please wait for @fhahn LGTM as well.

It looks like the patch was landed but the commit message seems a bit out-of-sync with the code.it

llvm/test/Transforms/LoopSimplify/update_latch_md2.ll

7

It would probably be good to check the while function using the auto-generation script.

[hgreving]

In D125574#3533949, @fhahn wrote:

In D125574#3532385, @Meinersbur wrote:

LGTM. Please wait for @fhahn LGTM as well.

It looks like the patch was landed but the commit message seems a bit out-of-sync with the code.it

Sorry I missed that request, but I had left it intentionally, the patch still does not move, but copies.

It would probably be good to check the while function using the auto-generation script.

Would you like me to amend?

llvm/lib/Transforms/Utils/BasicBlockUtils.cpp

UpdateAnalysisInformation(BB, NewBB, Preds, DTU, DT, LI, MSSAU, PreserveLCSSA,
                             HasLoopExit);
 
   if (!Preds.empty()) {
     // Update the PHI nodes in BB with the values coming from NewBB.
     UpdatePHINodes(BB, NewBB, Preds, BI, HasLoopExit);
   }
 
   if (OldLatch) {
+    Loop *IL = LI->getLoopFor(OldLatch);
+    // It's still possible that OldLatch is the latch of another inner loop, in
+    // which case we do not move the metadata.
+    if (IL && IL->getLoopLatch() != OldLatch) {
       BasicBlock *NewLatch = L->getLoopLatch();
       if (NewLatch != OldLatch) {
         MDNode *MD = OldLatch->getTerminator()->getMetadata("llvm.loop");
         NewLatch->getTerminator()->setMetadata("llvm.loop", MD);
         OldLatch->getTerminator()->setMetadata("llvm.loop", nullptr);
       }
     }
+  }
 
   return NewBB;
 }
 
 BasicBlock *llvm::SplitBlockPredecessors(BasicBlock *BB,
                                          ArrayRef<BasicBlock *> Preds,
                                          const char *Suffix, DominatorTree *DT,
                                          LoopInfo *LI, MemorySSAUpdater *MSSAU,

llvm/test/Transforms/LoopSimplify/update_latch_md2.ll

 ; Tests loop-simplify does not move the loop metadata, because
 ; the loopexit block is not the latch of the loop _bb6.
 
-; FIXME(#55416): The metadata should not move.
-
 ; RUN: opt < %s -passes=loop-simplify -S | FileCheck %s
 ; CHECK-LABEL: loop.header.loopexit:
-; CHECK: br label %loop.header, !llvm.loop !0
+; CHECK-NOT: br label %loop.header, !llvm.loop !0
 ; CHECK-LABEL: loop.latch:
-; CHECK-NOT: br i1 %p, label %loop.latch, label %loop.header.loopexit, !llvm.loop !0
+; CHECK: br i1 %p, label %loop.latch, label %loop.header.loopexit, !llvm.loop !0
 
 define void @func(i1 %p) {
 bb1:
   br label %loop.header
 
 loop.header:
   br i1 %p, label %bb2, label %_exit
 
 bb2:
   br i1 %p, label %bb4, label %bb3
 
 bb4:
   br label %bb3
 
 bb3:
   br label %loop.latch
 
 loop.latch:
   br i1 %p, label %loop.latch, label %loop.header, !llvm.loop !0
 
 _exit:
   ret void
 }
 
 !0 = distinct !{!0, !1}
 !1 = !{!"llvm.loop.mustprogress"}