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

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added subscribers: fhahn, hfinkel.
nickdesaulniers 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())
----------------
@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?


================
Comment at: llvm/test/CodeGen/X86/callbr-asm-outputs-pred-succ.ll:38
+; CHECK: bb.4 (%ir-block.11, address-taken):
+; CHECK-NEXT: predecessors: %bb.0
+
----------------
@void I recognize the irony of preprocessing the other test I modified, in child revision https://reviews.llvm.org/D77356, then adding a new test that doesn't do the same formatting.

I tend to write comments in my test of what's being tested that way in the future when someone else needs to change my test, they have some sense of what's important to the test and what's not.  That way they feel more empowered to change them.

Should I:
1. preprocess the test with update_llc_test_checks.py and then
2. remove these comments

I don't mind deleting anything, and trust your judgement, but I wanted to highlight this irony during code review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76961