void marked an inline comment as done.
void added inline comments.
================
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:
> 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?
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
