[PATCH] D130290: [MachineVerifier] add checks for INLINEASM_BR
Nick Desaulniers via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 21 10:45:21 PDT 2022
nickdesaulniers added inline comments.
================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:882
+
+ const BasicBlock *IndirectTarget = MO.getBlockAddress()->getBasicBlock();
+
----------------
arsenm wrote:
> Why is this based on the IR block?
Because AFAIK, there is no corresponding `BlockAddress` `Constant` in the MIR level, so the `MachineInst` uses a `BlockAddress` `Constant` which is just a tuple of (`Function`, `BasicBlock`). What would be helpful here would be if `BlockAddress` `Constants` were lowered to something new (doesn't exist, yet) that is a tuple of (`MachineFunction`, `MachineBasicBlock`).
The existing design seems brittle, because IIUC, there's a one-to-many relationship between `BasicBlock` and `MachineBasicBlock`. A `MachineBasicBlock` can refer to a single `BasicBlock`.
Perhaps that's why in my TODO below that we _don't_ have a mapping of `BasicBlock` to `MachineBasicBlock`; because they are not one-to-one (unless I'm wrong).
================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:899-904
+ if (llvm::none_of(MBB->successors(),
+ [IndirectTargetMBB](const MachineBasicBlock *Succ) {
+ return Succ == IndirectTargetMBB;
+ }))
+ report("INLINEASM_BR indirect target missing from successor list", &MO,
+ i);
----------------
arsenm wrote:
> arsenm wrote:
> > I'd expect CFG checks to go with the others in visitMachineBasicBlockBefore
> This is !MBB->isSuccessor
Perhaps, but consider that INLINEASM_BR are relatively rare constructs to see in the wild; I think it might be more efficient to do such scans only when they are encountered.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130290/new/
https://reviews.llvm.org/D130290
More information about the llvm-commits
mailing list