[PATCH] D64101: [LoopUnroll] fix cloning callbr

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
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));
----------------
fhahn wrote:
> 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?
> Is it possible for It->second to not be a BB here?

I'm pretty sure `It->second` can ONLY be a `BasicBlock` here, since the values stored in `VMap` were created by `llvm::CloneBasicBlock` (see `VMap` there).

That said, I'm not super confident in my understanding of `dyn_cast` vs `static_cast`.

The `VMap` is an instance of `ValueToValueMapTy`, which simply maps `Value*` to `Value*`.  As `Value` is the base class of a lot of stuff in LLVM, we end up storing all kinds of pointers to derived classes in `VMap`.

My understanding of when to use `dyn_cast` (or even `dynamic_cast`) vs `static_cast` is that it's always ok to `static_cast` from derived class to base class, but not vice versa.  That's why `dyn_cast` or `dynamic_cast` might result in `nullptr` at runtime, and thus need to be checked.

Since `It->second` should ALWAYS (IIUC) be a `BasicBlock*` in this case, I would have thought we still need a checked call to `dyn_cast`.

Maybe I'm misunderstanding and that it's ok to `static_cast` from base to derived if you're certain that base is always derived?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64101