[PATCH] D76961: [SelectionDAG] fix predecessor list for INLINEASM_BRs' parent

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:893
+    for (const MachineOperand &MO : MI->operands())
+      if (MO.isBlockAddress())
+        for (const MachineBasicBlock &MBB : *MI->getParent()->getParent())
----------------
nickdesaulniers wrote:
> @efriedma , so I'm hesitant about this added verification check.  As written, it's checking each operand of `INLINEASM_BR` that's a `blockaddress`, which again may or may not be the indirect target of `asm goto` (it could just be a vanilla input).
> 
> This goes back to our discussion in https://reviews.llvm.org/D64101#1569218 with @fhahn and @hfinkel .
> 
> At this point, I think I agree with your previous comment on this review (https://reviews.llvm.org/D76961#1950958), ie.
> 
> > Even if there isn't any way to tell the difference from the INLINEASM_BR instruction at the moment, we could change that.
> 
> I think we should just make `callbr` assume that any `blockaddress` operand is a possible successor.  The implication then is that it might then even be safe to use a vanilla asm statement (rather than `asm goto`), with labels as inputs, and get the same functionality as `asm goto`.  `asm goto` is kind of just a distinct syntax for what feels like could have been fixes to codegen for the original syntax.
> 
> I don't really want to discuss such a change here, but it's something for us to think more about and maybe discuss at the next LLVM developer meeting, or on the mailing list.
> 
> I'm more curious about @efriedma 's thoughts on how to proceed:
> 1. drop this verification pass, defer decisions to the future, or
> 2. keep this pass, and add the additional check I commented earlier in this thread.
> 
> I like the idea of 2, but I'm concerned that if we add a verification pass, then someone tries to use blockaddress operands in callbr that are not indirect successors, we probably don't set the successor list correctly, and this added verification check will be too aggressive and error.
> 
> So maybe it's better to drop the the pass, and revisit this another day, when were confident in the direct and changes we'd like to make.
> 
> Thoughts?
I'm fine with just dropping the verifier check.  Jump table jumps have implicit targets in a similar way, so unverifiable CFGs aren't really anything novel.

-----

Way off-topic response to the general design of the feature:

At the IR level, at this point, I think that using blockaddress for callbr was a mistake.  The list of valid destinations is already listed in the "indirect destinations" list of the callbr; we could just allow asm strings to refer to that label list directly.  This would be a little less flexible from a source language perspective, since you couldn't pass the address of a label as a register operand, but at the C level I don't think anyone is actually passing &&LABEL to asm goto in practice.

> asm goto is kind of just a distinct syntax for what feels like could have been fixes to codegen for the original syntax.

If the syntax isn't distinct, we would be forced to impose indirect goto's "jump-into-scope" restrictions on all inline asm statements.  Probably this would break someone's code.

I guess if we were designing the feature from scratch, we could allow asm goto blocks to jump to any block in the function whose address is taken.  But that isn't any more powerful than the feature we currently implement.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:897
+            SmallPtrSet<const MachineBasicBlock *, 2> Preds;
+            for (const MachineBasicBlock *Pred : MBB.predecessors())
+              Preds.insert(Pred);
----------------
is_contained?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76961