void marked 2 inline comments 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
----------------
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. :-)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D69868/new/
https://reviews.llvm.org/D69868
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
