[PATCH] D69868: Allow "callbr" to return non-void values

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers accepted this revision.
nickdesaulniers added a comment.
This revision is now accepted and ready to land.

This code has now been tested on a running Linux kernel making use of the feature.

I still would like @jyknight to clarify his comments, consider explicitly requesting changes to this CL, or consider resigning as reviewer.



================
Comment at: llvm/include/llvm/CodeGen/MachineBasicBlock.h:137
+  /// List of indirect targets of the callbr of a basic block.
+  SmallPtrSet<const MachineBasicBlock*, 4> InlineAsmBrIndirectTargets;
+
----------------
It's likely the count here is 0, or maybe 1.  We don't see too often a large list of labels here.


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:701
+      } else if (MBB->succ_size() == LandingPadSuccs.size() ||
+                 MBB->succ_size() == IndirectPadSuccs.size()) {
         // It's possible that the block legitimately ends with a noreturn
----------------
void wrote:
> void wrote:
> > jyknight wrote:
> > > void wrote:
> > > > jyknight wrote:
> > > > > This isn't correct.
> > > > > 
> > > > > This line here, is looking at a block which doesn't end in a jump to a successor. So, it's trying to verify that the successor list makes sense in that context.
> > > > > 
> > > > > The unstated assumption in the code is that the only successors will be landing pads. Instead of actually checking each one, instead it just checks that the count is the number of landing pads, with the assumption that all the successors should be landing pads, and that all the landing pads should be successors.
> > > > > 
> > > > > The next clause is then checking for the case where there's a fallthrough to the next block. In that case, the successors should've been all the landing pads, and the single fallthrough block.
> > > > > 
> > > > > Adding similar code to check for the number of callbr targets doesn't really make sense. It's certainly not the case that all callbr targets are targets of all 
> > > > > callbr instructions. And even if it was, this still wouldn't be counting things correctly.
> > > > > 
> > > > > 
> > > > > However -- I think i'd expect analyzeBranch to error out (returning true) when confronted by a callbr instruction, because it cannot actually tell what's going on there. If that were the case, nothing in this block should even be invoked. But I guess that's probably not happening, due to the terminator being followed by non-terminators.
> > > > > 
> > > > > That seems likely to be a problem that needs to be fixed. (And if that is fixed, I think the changes here aren't needed anymore)
> > > > > 
> > > > > 
> > > > Your comment is very confusing. Could you please give an example of where this fails?
> > > Sorry about that, I should've delimited the parts of that message better...
> > > Basically:
> > > - Paragraphs 2-4 are describing why the code before this patch appears to be correct for landing pad, even though it's taking some shortcuts and making some non-obvious assumptions.
> > > - Paragraph 5 ("Adding similar code"...) is why it's not correct for callbr.
> > > - Paragraph 6-7 are how I'd suggest to resolve it.
> > > 
> > > 
> > > I believe the code as of your patch will fail validation if you have a callbr instruction which has a normal-successor block which is an indirect target of a *different* callbr in the function.
> > > 
> > > I believe it'll also fail if you have any landing-pad successors, since those aren't being added to the count of expected successors, but rather checked separately.
> > > 
> > > But more seriously than these potential verifier failures, I expect that analyzeBranch returning wrong answers (in that it may report that a block unconditionally-jumps to a successor, while it really has both a callbr and jump, separated by the non-terminator copies) will cause miscompilation. I'm not sure exactly how that will exhibit, but I'm pretty sure it's not going to be good.
> > > 
> > > And, if analyzeBranch properly said "no idea" when confronted by callbr control flow, then this code in the verifier wouldn't be reached.
> > I didn't need a delineation of the parts of the comment. I needed a clearer description of what your concern is, and to give an example of code that fails here.
> > 
> > This bit of code is simply saying that if the block containing the `INLINEASM_BR` doesn't end with a `BR` instruction, then the number of its successors should be equal to the number of indirect successors. This is correct, as it's not valid to have a duplicate label used in a `callbr` instruction:
> > 
> > ```
> > $ llc -o /dev/null x.ll
> > Duplicate callbr destination!
> >   %3 = callbr i32 asm sideeffect "testl $0, $0; testl $1, $1; jne ${2:l}", "={si},r,X,0,~{dirflag},~{fpsr},~{flags}"(i32 %2, i8* blockaddress(@test1, %asm.fallthrough), i32 %1) #2
> >           to label %asm.fallthrough [label %asm.fallthrough], !srcloc !6
> > ./bin/llc: x.ll: error: input module is broken!
> > ```
> > 
> > A `callbr` with a normal successor block that is the indirect target of a different `callbr` isn't really relevant here, unless I'm misunderstanding what `analyzeBranch` returns. There would be two situations:
> > 
> > 1. The MBB ends in a fallthrough, which is the case I mentioned above, or
> > 
> > 2. The MBB ends in a `BR` instruction, in which case it won't be in this block of code, but the block below.
> > 
> > If `analyzeBranch` is not taking into account potential `COPY` instructions between `INLINEASM_BR` and `BR`, then it needs to be addressed there (I'll verify that it is). I *do* know that this code is reached by the verifier, so it handles it to some degree. :-)
> > But more seriously than these potential verifier failures, I expect that
> > analyzeBranch returning wrong answers (in that it may report that a block 
> > unconditionally-jumps to a successor, while it really has both a callbr and 
> > jump, separated by the non-terminator copies) will cause miscompilation. I'm 
> > not sure exactly how that will exhibit, but I'm pretty sure it's not going 
> > to be good.
> 
> Here are two proposals that may help alleviate these concerns:
> 
> 1. Have analyzeBranch skip over the COPYs between the INLINEASM_BR and the JMP. It's relatively straight-forward to do, but it would have to be done for *all* analyzeBranch calls.
> 
> 2. Create a new pseudo-instruction called `INLINEASM_BR_COPY` (or some better name) that's a terminator which behaves like a normal `COPY`, but the analyzeBranch and other methods that look at terminators will be able to handle it without modifications, since it'll look similarly to an `INLINEASM_BR` instruction. It doesn't require changing all of analyzeBranch implementations, but it's a much larger change.
> 
> Thoughts?
> Instead of actually checking each one, instead it just checks that the count is the number of landing pads, with the assumption that all the successors should be landing pads, and that all the landing pads should be successors.

What do you mean "instead of actually checking each one?" What check should be done?

> It's certainly not the case that all callbr targets are targets of all callbr instructions. And even if it was, this still wouldn't be counting things correctly.

Right, so you could have a `MachineBasicBlock` that's the target of a `INLINEASM_BR`, and a different `MachineBasicBlock` that also branches to the `INLINEASM_BR` target (but itself wasn't an `INLINEASM_BR`). But `IndirectTargetSuccs` is only built up from successors of the current block that are `isInlineAsmBrIndirectTarget`'s.  So I don't understand how the count is "wrong" just because you could have other MBB's also target the current MBB's indirect successor.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:1076
+    // to the copy so that everyone is happy.
+    for (auto *Succ : BB->successors())
+      if (Succ != CopyBB && !CopyBB->isSuccessor(Succ))
----------------
Isn't `Fallthrough` from above one of the potential successors? Do we have to skip it in the below conditional? What happens if we call `addSuccessor` with the same `MBB` twice?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69868