[PATCH] D67252: [IR] CallBrInst: scan+update arg list when indirect dest list changes

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers marked 2 inline comments as done.
nickdesaulniers added inline comments.


================
Comment at: llvm/include/llvm/IR/Instructions.h:4085
   BasicBlock *getIndirectDest(unsigned i) const {
-    return cast<BasicBlock>(*(&Op<-1>() - getNumIndirectDests() + i));
+    return cast_or_null<BasicBlock>(*(&Op<-1>() - getNumIndirectDests() + i));
   }
----------------
craig.topper wrote:
> Why is this change needed?
This change requires careful analysis of `CallBrInst::init`.  `CallBrInst::init` calls `CallBrInst::setIndirectDest` while initializing the instruction.  Calling `CallBrInst::getIndirectDest` via `CallBrInst::updateArgBlockAddresses` via `CallBrInst::setIndirectDest` leads to a compiler crash (IIUC, it's not safe to pass `cast` a `nullptr`).  Essentially, it's not safe to `getIndirectDest` before `setIndirectDest` without this change.


================
Comment at: llvm/lib/IR/Instructions.cpp:827
+  assert(getNumIndirectDests() > i && "IndirectDest # out of range for callbr");
+  if (BasicBlock *OldBB = getIndirectDest(i)) {
+    BlockAddress *Old = BlockAddress::get(OldBB);
----------------
craig.topper wrote:
> When would this fail?
There's two scenarios that could call `CallBrInst::updateArgBlockAddresses` via `CallBrInst::setIndirectDest`.

1. We're initializing the instruction.  `CallBrInst::init` calls `CallBrInst::setIndirectDest` calls `CallBrInst::updateArgBlockAddresses`.  There is no `OldBB`, because we're initializing the instruction, so there's nothing to do.
2. We're updating the successor.  `CallBrInst::setSuccessor` now calls `CallBrInst::setIndirectDest` calls `CallBrInst::updateArgBlockAddresses`.  Now there is an `OldBB` which implies there should be a `BlockAddress` `Constant` argument operand we need to update as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67252