nickdesaulniers added inline comments.

================
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]
 
----------------
void wrote:
> 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`.
> I'm not sure how it could have worked, given that LLVM's tools should abort 
> when it sees the bad blockaddress

The function referred to in the `blockaddress` does not need to be scoped to 
the same function; otherwise I don't think you could store the address of a 
label in a global variable (without shenanigans like converting it to a 
integer).

>  IOW, it should be @bar.

```
diff --git a/llvm/test/tools/llvm-diff/callbr.ll 
b/llvm/test/tools/llvm-diff/callbr.ll
index f925606c11cf..8a26f3529d43 100644
--- a/llvm/test/tools/llvm-diff/callbr.ll
+++ b/llvm/test/tools/llvm-diff/callbr.ll
@@ -25,7 +25,7 @@ return:
 
 define void @bar() {
 entry:
-  callbr void asm sideeffect "", "i,i,~{dirflag},~{fpsr},~{flags}"(i8* 
blockaddress(@foo, %t_no), i8* blockaddress(@foo, %return))
+  callbr void asm sideeffect "", "i,i,~{dirflag},~{fpsr},~{flags}"(i8* 
blockaddress(@bar, %t_no), i8* blockaddress(@bar, %return))
           to label %asm.fallthrough [label %return, label %t_no]
 
 asm.fallthrough:
```
causes the test to fail as there's no output from `llvm-diff` to input to 
`FileCheck`. What was the original intent of 
`llvm/test/tools/llvm-diff/callbr.ll`?


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

Reply via email to