This is an archive of the discontinued LLVM Phabricator instance.

[llvm][test] rewrite callbr to use i rather than X constraint NFC
ClosedPublic

Authored by nickdesaulniers on Dec 8 2021, 4:57 PM.

Details

Summary

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

In D115311, we're looking to modify clang to emit i constraints rather
than X constraints for callbr's indirect destinations. Prior to doing
so, update all of the existing tests in llvm/ to match.

Diff Detail

Event Timeline

nickdesaulniers created this revision.Dec 8 2021, 4:57 PM
nickdesaulniers requested review of this revision.Dec 8 2021, 4:57 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 8 2021, 4:57 PM
nickdesaulniers edited the summary of this revision. (Show Details)Dec 8 2021, 5:11 PM
nickdesaulniers added inline comments.Dec 8 2021, 5:44 PM
llvm/test/Verifier/callbr.ll
41

Note to reviewers, this is the only case that was interesting IMO, because the middle input operand is not an indirect destination. Ie. the label referred to in the blockaddress parameter is not duplicated in the [] on the next line.

Not that it makes a difference, just preemptively answering the question: "why is this case different from everything else in this change?"

pengfei added inline comments.Dec 8 2021, 7:39 PM
llvm/test/Verifier/callbr.ll
42

Does it mean we are able to pass blockaddress out of current function? For that case, we can't put it in the indirect labels list, right?

llvm/test/tools/llvm-diff/callbr.ll
28–29

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?

This looks good to me, but I'd like to wait for a conclusion on D115409 first.

jyknight added inline comments.Dec 13 2021, 3:13 PM
llvm/test/Verifier/callbr.ll
42

In general, you may pass whatever blockaddress you like to an asm -- it's just a value. You just cannot jump to a location that's not it's listed in the indirect labels list.

llvm/test/tools/llvm-diff/callbr.ll
28–29

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.)

pengfei added inline comments.Dec 13 2021, 7:57 PM
llvm/test/tools/llvm-diff/callbr.ll
28–29

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.

nickdesaulniers marked 3 inline comments as done.Dec 14 2021, 4:35 PM
nickdesaulniers added inline comments.
llvm/test/tools/llvm-diff/callbr.ll
28–29

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.

jyknight added inline comments.Dec 16 2021, 3:05 PM
llvm/test/tools/llvm-diff/callbr.ll
28–29

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.

nickdesaulniers marked an inline comment as done.
nickdesaulniers added inline comments.
llvm/test/tools/llvm-diff/callbr.ll
28–29

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 blockaddresses 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.

void accepted this revision.Dec 21 2021, 2:42 PM

Approved with one change.

llvm/test/tools/llvm-diff/callbr.ll
28–29

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.

This revision is now accepted and ready to land.Dec 21 2021, 2:42 PM
llvm/test/tools/llvm-diff/callbr.ll
28–29

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?

jyknight accepted this revision.Jan 7 2022, 11:50 AM
This revision was landed with ongoing or failed builds.Jan 11 2022, 11:31 AM
This revision was automatically updated to reflect the committed changes.