[PATCH] D66892: [BasicBlockUtils] Add metadata fixing in SplitBlockPredecessors.

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

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



================
Comment at: llvm/test/Transforms/LoopSimplify/loop_metadata.ll:1
+; RUN: opt -S -loop-simplify < %s | FileCheck %s
+
----------------
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.



================
Comment at: llvm/test/Transforms/LoopSimplify/loop_metadata.ll:4
+; CHECK: for.cond.loopexit:
+; CHECK: br label %for.cond, !llvm.loop !0
+; CHECK: br i1 %cmp1, label %for.body1, label %for.cond.loopexit
----------------
I think this should be a CHECK-NEXT to ensure this branch is part of the `for.cond.loopexit` BB.


================
Comment at: llvm/test/Transforms/LoopSimplify/loop_metadata.ll:5
+; CHECK: br label %for.cond, !llvm.loop !0
+; CHECK: br i1 %cmp1, label %for.body1, label %for.cond.loopexit
+
----------------
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{{$}}




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D66892/new/

https://reviews.llvm.org/D66892