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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
