[PATCH] D115410: [llvm][test] rewrite callbr to use i rather than X constraint NFC

[Bill Wendling via Phabricator via cfe-commits]

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