[test] Fix coro-split-dbg-labels.ll for non-x86

[vogelsgesang]

This test case was recently introduced in 24b3c1f. It was meant to be target-independent, but the function attributes still contained x86-specific attributes. This commit removes those attributes.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-coroutines

Author: Adrian Vogelsgesang (vogelsgesang)

Changes

This test case was recently introduced in 24b3c1f. It was meant to be target-independent, but the function attributes still contained x86-specific attributes. This commit removes those attributes.

---

Full diff: https://github.com/llvm/llvm-project/pull/147087.diff

1 Files Affected:

• (modified) llvm/test/Transforms/Coroutines/coro-split-dbg-labels.ll (+11-14)

diff --git a/llvm/test/Transforms/Coroutines/coro-split-dbg-labels.ll b/llvm/test/Transforms/Coroutines/coro-split-dbg-labels.ll
index c8cea268d6778..25c6316388510 100644
--- a/llvm/test/Transforms/Coroutines/coro-split-dbg-labels.ll
+++ b/llvm/test/Transforms/Coroutines/coro-split-dbg-labels.ll
@@ -36,7 +36,7 @@ entry:
   br label %loop1, !dbg !27
 
 loop1:                                         ; preds = %for.cond, %entry
-  tail call void (...) @bar() #7, !dbg !33
+  tail call void (...) @bar() #0, !dbg !33
   %3 = tail call token @llvm.coro.save(ptr null), !dbg !34
   %4 = tail call i8 @llvm.coro.suspend(token %3, i1 false), !dbg !34
   switch i8 %4, label %coro_Suspend [
@@ -45,7 +45,7 @@ loop1:                                         ; preds = %for.cond, %entry
   ], !dbg !34
 
 loop2:                                         ; preds = %for.cond, %entry
-  tail call void (...) @baz() #7, !dbg !35
+  tail call void (...) @baz() #0, !dbg !35
   %5 = tail call token @llvm.coro.save(ptr null), !dbg !36
   %6 = tail call i8 @llvm.coro.suspend(token %5, i1 false), !dbg !36
   switch i8 %6, label %coro_Suspend [
@@ -97,24 +97,21 @@ coro_Suspend:                                     ; preds = %for.cond, %if.then,
 declare token @llvm.coro.id(i32, ptr readnone, ptr nocapture readonly, ptr) #5
 
 ; Function Attrs: nounwind
-declare noalias ptr @malloc(i64) local_unnamed_addr #6
+declare noalias ptr @malloc(i64) local_unnamed_addr #0
 declare i64 @llvm.coro.size.i64() #1
-declare ptr @llvm.coro.begin(token, ptr writeonly) #7
-declare token @llvm.coro.save(ptr) #7
-declare i8 @llvm.coro.suspend(token, i1) #7
+declare ptr @llvm.coro.begin(token, ptr writeonly) #0
+declare token @llvm.coro.save(ptr) #0
+declare i8 @llvm.coro.suspend(token, i1) #0
 declare ptr @llvm.coro.free(token, ptr nocapture readonly) #5
-declare void @free(ptr nocapture) local_unnamed_addr #6
-declare i1 @llvm.coro.end(ptr, i1, token) #7
+declare void @free(ptr nocapture) local_unnamed_addr #0
+declare i1 @llvm.coro.end(ptr, i1, token) #0
 
-attributes #0 = { nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "frame-pointer"="none" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #0 = { nounwind }
 attributes #1 = { nounwind readnone }
-attributes #2 = { "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "frame-pointer"="none" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
-attributes #3 = { nounwind uwtable presplitcoroutine "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "frame-pointer"="none" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #2 = {}
+attributes #3 = { nounwind uwtable presplitcoroutine }
 attributes #4 = { noduplicate }
 attributes #5 = { argmemonly nounwind readonly }
-attributes #6 = { nounwind "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "frame-pointer"="none" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
-attributes #7 = { nounwind }
-attributes #8 = { alwaysinline nounwind }
 
 !llvm.dbg.cu = !{!0}
 !llvm.module.flags = !{!3, !4}

[rorth]

You should have selected reviewers to get attention for this PR.

No idea why the CI Checks failed: seems like an internal error unrelated to the patch to me.

However, when I tested the patch locally on both sparcv9-sun-solaris2.11 and amd64-pc-solaris2.11, it FAILed in both cases:

/var/llvm/local-sparcv9-release-stage2-A-flang-openmp/tools/clang/stage2-bins/bin/opt < /vol/llvm/src/llvm-project/local/llvm/test/Transforms/Coroutines/coro-split-dbg-labels.ll -passes='cgscc(coro-split)' -S | /var/llvm/local-sparcv9-release-stage2-A-flang-openmp/tools/clang/stage2-bins/bin/FileCheck /vol/llvm/src/llvm-project/local/llvm/test/Transforms/Coroutines/coro-split-dbg-labels.ll # RUN: at line 4
+ /var/llvm/local-sparcv9-release-stage2-A-flang-openmp/tools/clang/stage2-bins/bin/opt '-passes=cgscc(coro-split)' -S
+ /var/llvm/local-sparcv9-release-stage2-A-flang-openmp/tools/clang/stage2-bins/bin/FileCheck /vol/llvm/src/llvm-project/local/llvm/test/Transforms/Coroutines/coro-split-dbg-labels.ll
/var/llvm/local-sparcv9-release-stage2-A-flang-openmp/tools/clang/stage2-bins/bin/opt: <stdin>:111:1: error: attribute group has no attributes
attributes #2 = {}
^
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /var/llvm/local-sparcv9-release-stage2-A-flang-openmp/tools/clang/stage2-bins/bin/FileCheck /vol/llvm/src/llvm-project/local/llvm/test/Transforms/Coroutines/coro-split-dbg-labels.ll

Something is still amiss here.

[ChuanqiXu9]

I don't have knowledge for these target related things. But the change itself looks good.

[pogo59]

LGTM. Let's see if this fixes the bots.

[rorth]

LGTM. Let's see if this fixes the bots.

Please don't commit this as is: this will not only not fix the affected bots, it will break all others. I've since verified that the current version of the patch FAILs on x86_64-pc-linux-gnu in the same ways as on Solaris: the test is syntactically wrong!

[pogo59]

LGTM. Let's see if this fixes the bots.

Please don't commit this as is: this will not only not fix the affected bots, it will break all others. I've since verified that the current version of the patch FAILs on x86_64-pc-linux-gnu in the same ways as on Solaris: the test is syntactically wrong!

Oops! I should remember to check the CI before approving...

[asb]

FYI this has broken the RISC-V bots as well. If a fix to the test isn't obvious, I think temporarily removing this test to get the bots green is sensible.

But I think a fix is straightforward. LIke a number of the other tests in llvm/tests/Transforms/Coroutines have, I suspect that the simplest solution is to add the x86 target-triple and datalayout to the input file. We should really have REQUIRES: x86-registered-target as well, but it seems tests in this directory are bad at providing it (and all of the bots are building the X86 target I assume).

[pogo59]

(and all of the bots are building the X86 target I assume)

Not all--I'm pretty sure there are ARM-only bots--but most.

[vogelsgesang]

Sorry for the confusion, this PR was not yet quite ready for review, yet. I primarily posted it to get feedback from CI. I should have pointed that out more clearly.

Something is still amiss here.

Should be fixed now

[rorth]

Sorry for the confusion, this PR was not yet quite ready for review, yet. I primarily posted it to get feedback from CI. I should have pointed that out more clearly.

Indeed: this PR didn't go as smoothly as it could have ;-(

Something is still amiss here.

Should be fixed now

Indeed: this version PASSes on all of sparcv9-sun-solaris2.11, amd64-pc-solaris2.11, and x86_64-pc-linux-gnu.

LGTM if the CI passes, too.

[vogelsgesang]

We should really have REQUIRES: x86-registered-target as well, but it seems tests in this directory are bad at providing it (and all of the bots are building the X86 target I assume).

Note that most test cases in llvm/tests/Transforms/Coroutines don't actually invoke the codegen pipeline.
They only call opt to invoke the coro{split,early,...} passes. The test cases are actually target-independent.
Afaict, that's the reason why they currently pass also on RISCV, Arm and other bots.

Adding REQUIRES: x86-registered-target would go in the wrong direction, as it would reduce test coverage on non-x86 targets. Instead, we should simply remove the target {datalayout,triple}, e.g., from llvm/test/Transforms/Coroutines/coro-async-declaration.ll (and many others) to avoid this source of confusion.

I suspect that the simplest solution is to add the x86 target-triple and datalayout to the input file.

In contrast to most other coroutine test cases, coro-split-dbg-labels.ll cannot actually use the x86 target-triple to also test the corosplit pass on RISCV/ARM/... because this test case not only calls opt. It also calls %llc_dwarf to actually compile the LLVM IR to code. It does so to verify that the generated DWARF debug info contains the expected entries

[vogelsgesang]

This time around, the Linux and Windows pre-commit are actually green. I couldn't verify RISC-V and Sparc, since I don't have access to any such machines.

@rorth @asb Do you want to verify locally that this PR actually fixes the issue before I merge this? Or do you want me to land the patch as is? (hoping that it fixes the issue - in the worst case, it at least won't regress the current situation)

[rorth]

@rorth @asb Do you want to verify locally that this PR actually fixes the issue before I merge this? Or do you want me to land the patch as is? (hoping that it fixes the issue - in the worst case, it at least won't regress the current situation)

I already mentioned that the latest version of this PR fixes the SPARC failure and works on both Solaris/amd64 and Linux/x86_64. I don't think there's anything blocking the merge: I'd rather see it happen sooner than later, given that the Solaris/sparcv9 bot has been red for 4 days now.

[asb]

Yes please land, thank you!