void accepted this revision.
void added a comment.
This revision is now accepted and ready to land.
Approved with one change.
================
Comment at: llvm/test/tools/llvm-diff/callbr.ll:28-29
entry:
- callbr void asm sideeffect "", "X,X,~{dirflag},~{fpsr},~{flags}"(i8*
blockaddress(@foo, %t_no), i8* blockaddress(@foo, %return))
+ callbr void asm sideeffect "", "i,i,~{dirflag},~{fpsr},~{flags}"(i8*
blockaddress(@foo, %t_no), i8* blockaddress(@foo, %return))
to label %asm.fallthrough [label %return, label %t_no]
----------------
nickdesaulniers wrote:
> jyknight wrote:
> > nickdesaulniers wrote:
> > > pengfei wrote:
> > > > jyknight wrote:
> > > > > pengfei wrote:
> > > > > > If my above assumption is true, I think we can't replace the `X`
> > > > > > with `i` here.
> > > > > > Besides, I'm confused on the indirect labels list. Are they the
> > > > > > labels of `bar` or `foo`?
> > > > > I don't see a a problem with using "i" everywhere -- all blockaddress
> > > > > are going to be immediate values no matter whether they're an
> > > > > indirect target or not.
> > > > >
> > > > > The indirect labels list is only referring to labels in the current
> > > > > function.
> > > > >
> > > > > This test is confusing, but, it is a test for llvm-diff, so that's
> > > > > okay or maybe even intended. (It can't actually possibly jump to
> > > > > @bar:%return or @bar:%t_no, because nothing ever gets the address of
> > > > > those labels. It does get the similarly-named labels in @foo, but it
> > > > > can't jump to those either, since they're in a different function.)
> > > > Thanks for the explanation. My point is the test3 above intended to use
> > > > `X` to indicate the destination is not in the indirect labels list. For
> > > > consistency, we should use `X` here too, since the @foo:%return etc.
> > > > are not in the list either. Or we don't need to use `X` in test3.
> > > The "indirect destination list" for the `callbr` in `@bar` is the `[label
> > > %return, label %t_no]`. Both operands have corresponding `blockaddress`
> > > arguments. So they //should not// use `X` in this case.
> > I don't see why the correct constraint to use should be related at all to
> > whether the blockaddress argument corresponds to a label in the indirect
> > label list or not.
> >
> > Using something other than "X" should probably always be preferred, since
> > presumably the instruction you're emitting has requirements. (Unless of
> > course you don't actually use the argument, or only use it in a comment, or
> > something like that...in which case "X" is fine.)
> >
> > But, FTR, in this test, the blockaddress is for a label in a //different//
> > function ("@foo") than the function we're in ("@bar"), which is what
> > pengfei was pointing out.
> > I don't see why the correct constraint to use should be related at all to
> > whether the blockaddress argument corresponds to a label in the indirect
> > label list or not.
> > Using something other than "X" should probably always be preferred
>
> Note: child patch D115311 only changes the goto label list for `asm goto`; it
> does not change labels in the input list.
>
> > But, FTR, in this test, the blockaddress is for a label in a different
> > function ("@foo") than the function we're in ("@bar"), which is what
> > pengfei was pointing out.
>
> Ah, I missed that. @void can you clarify; `@bar` looks exactly like `@foo`
> to me; was `@bar` copy-pasted from `@foo` without the `blockaddress`es being
> updated to refer to `@bar`? I don't really understand the intent of this
> test; I don't understand why `llvm-diff` has output for `@bar` but not
> `@foo`. If there's a difference, I'm having trouble spotting it.
It's a copy-and-paste error, and I'm not sure how it could have worked, given
that LLVM's tools should abort when it sees the bad `blockaddress`. IOW, it
should be `@bar`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D115410/new/
https://reviews.llvm.org/D115410
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
