[PATCH] D64101: [LoopUnroll] fix cloning callbr

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:88
+
+    if (auto *BA = dyn_cast<BlockAddress>(Op)) {
+      ValueToValueMapTy::iterator It = VMap.find(BA->getBasicBlock());
----------------
IIUC, for block addresses, we now set the operand first by the code above and then also by this block here. It might be slightly better to first check for the special case and `continue` in case we hit it. 

Also, a brief comment why we need special handling here would be helpful IMO.


================
Comment at: llvm/lib/Transforms/Utils/LoopUnroll.cpp:91
+      if (It != VMap.end() && isa<CallBrInst>(*I))
+        if (auto *NewBB = dyn_cast<BasicBlock>(It->second))
+          I->setOperand(OpNo, BlockAddress::get(NewBB));
----------------
Is it possible for It->second to not be a BB here? I think if the original operand was a BB, the new one also should be a BB. So this could be a regular cast?


================
Comment at: llvm/test/Transforms/LoopUnroll/callbr.ll:1
+; RUN: opt -loop-unroll -S -o - %s 2>&1 | FileCheck %s
+
----------------
The test only checks the emitted IR and no debug output. So the 2>&1 should not be needed, right?


================
Comment at: llvm/test/Transforms/LoopUnroll/callbr.ll:23
+for.cond.cleanup:                                 ; preds = %for.inc
+  ret i32 undef
+
----------------
nit: could just be a void function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64101