[RS4GC] Fix some test comments that got clobbered #145186
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The commit 0407108 did some transformations on tests for RewriteStatepointsForGC that put some comments out-of-order. At least the `basics.ll` test is referred to by the current LLVM statepoints documentation, so this may have caused confusion to end-users investigating further into statepoint usage. Additionally, some comments were outright removed that explained potentially non-trivial behaviour of the pass in the tests.
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
|
@llvm/pr-subscribers-llvm-transforms Author: Wren [Undefined] (ThePuzzlemaker) ChangesThe commit 0407108 did some transformations on tests for RewriteStatepointsForGC that put some comments out-of-order. At least the Additionally, some comments were outright removed that explained potentially non-trivial behaviour of the pass in the tests. This is my first time ever submitting a patch to LLVM, so please let me know if there's anything I accidentally overlooked. I re-tested all tests to ensure no issues accidentally arose from these changes. Full diff: https://github.com/llvm/llvm-project/pull/145186.diff 7 Files Affected:
diff --git a/llvm/test/Transforms/RewriteStatepointsForGC/base-vector.ll b/llvm/test/Transforms/RewriteStatepointsForGC/base-vector.ll
index b3ac71ac18db9..a6235e28a823a 100644
--- a/llvm/test/Transforms/RewriteStatepointsForGC/base-vector.ll
+++ b/llvm/test/Transforms/RewriteStatepointsForGC/base-vector.ll
@@ -92,8 +92,6 @@ entry:
}
define ptr addrspace(1) @test4(ptr addrspace(1) %ptr) gc "statepoint-example" {
-; When we can optimize an extractelement from a known
-; index and avoid introducing new base pointer instructions
; CHECK-LABEL: @test4(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[DERIVED:%.*]] = getelementptr i64, ptr addrspace(1) [[PTR:%.*]], i64 16
@@ -120,8 +118,9 @@ entry:
declare void @use(ptr addrspace(1)) "gc-leaf-function"
declare void @use_vec(<4 x ptr addrspace(1)>) "gc-leaf-function"
+; When we can optimize an extractelement from a known
+; index and avoid introducing new base pointer instructions
define void @test5(i1 %cnd, ptr addrspace(1) %obj) gc "statepoint-example" {
-; When we fundementally have to duplicate
; CHECK-LABEL: @test5(
; CHECK-NEXT: entry:
; CHECK-NEXT: [[GEP:%.*]] = getelementptr i64, ptr addrspace(1) [[OBJ:%.*]], i64 1
@@ -144,9 +143,8 @@ entry:
ret void
}
+; When we fundementally have to duplicate
define void @test6(i1 %cnd, ptr addrspace(1) %obj, i64 %idx) gc "statepoint-example" {
-; A more complicated example involving vector and scalar bases.
-; This is derived from a failing test case when we didn't have correct
; insertelement handling.
; CHECK-LABEL: @test6(
; CHECK-NEXT: entry:
@@ -170,6 +168,8 @@ entry:
ret void
}
+; A more complicated example involving vector and scalar bases.
+; This is derived from a failing test case when we didn't have correct
define ptr addrspace(1) @test7(i1 %cnd, ptr addrspace(1) %obj, ptr addrspace(1) %obj2) gc "statepoint-example" {
; CHECK-LABEL: @test7(
; CHECK-NEXT: entry:
@@ -384,4 +384,4 @@ bb1: ; preds = %bb1, %bb
br label %bb1
}
-declare i32 @spam() gc "statepoint-example"
\ No newline at end of file
+declare i32 @spam() gc "statepoint-example"
diff --git a/llvm/test/Transforms/RewriteStatepointsForGC/basics.ll b/llvm/test/Transforms/RewriteStatepointsForGC/basics.ll
index 644baebcca7ee..6d7746b31f056 100644
--- a/llvm/test/Transforms/RewriteStatepointsForGC/basics.ll
+++ b/llvm/test/Transforms/RewriteStatepointsForGC/basics.ll
@@ -1,21 +1,20 @@
; This is a collection of really basic tests for gc.statepoint rewriting.
; RUN: opt < %s -passes=rewrite-statepoints-for-gc -spp-rematerialization-threshold=0 -S | FileCheck %s
-; Trivial relocation over a single call
-
declare void @foo()
+; Trivial relocation over a single call
define ptr addrspace(1) @test1(ptr addrspace(1) %obj) gc "statepoint-example" {
; CHECK-LABEL: @test1
entry:
; CHECK-LABEL: entry:
; CHECK-NEXT: gc.statepoint
; CHECK-NEXT: %obj.relocated = call coldcc ptr addrspace(1)
-; Two safepoints in a row (i.e. consistent liveness)
call void @foo() [ "deopt"(i32 0, i32 -1, i32 0, i32 0, i32 0) ]
ret ptr addrspace(1) %obj
}
+; Two safepoints in a row (i.e. consistent liveness)
define ptr addrspace(1) @test2(ptr addrspace(1) %obj) gc "statepoint-example" {
; CHECK-LABEL: @test2
entry:
@@ -24,12 +23,12 @@ entry:
; CHECK-NEXT: %obj.relocated = call coldcc ptr addrspace(1)
; CHECK-NEXT: gc.statepoint
; CHECK-NEXT: %obj.relocated2 = call coldcc ptr addrspace(1)
-; A simple derived pointer
call void @foo() [ "deopt"(i32 0, i32 -1, i32 0, i32 0, i32 0) ]
call void @foo() [ "deopt"(i32 0, i32 -1, i32 0, i32 0, i32 0) ]
ret ptr addrspace(1) %obj
}
+; A simple derived pointer
define i8 @test3(ptr addrspace(1) %obj) gc "statepoint-example" {
entry:
; CHECK-LABEL: entry:
@@ -39,8 +38,6 @@ entry:
; CHECK-NEXT: %derived.relocated = call coldcc ptr addrspace(1)
; CHECK-NEXT: load i8, ptr addrspace(1) %derived.relocated
; CHECK-NEXT: load i8, ptr addrspace(1) %obj.relocated
-; Tests to make sure we visit both the taken and untaken predeccessor
-; of merge. This was a bug in the dataflow liveness at one point.
%derived = getelementptr i8, ptr addrspace(1) %obj, i64 10
call void @foo() [ "deopt"(i32 0, i32 -1, i32 0, i32 0, i32 0) ]
%a = load i8, ptr addrspace(1) %derived
@@ -49,6 +46,8 @@ entry:
ret i8 %c
}
+; Tests to make sure we visit both the taken and untaken predeccessor
+; of merge. This was a bug in the dataflow liveness at one point.
define ptr addrspace(1) @test4(i1 %cmp, ptr addrspace(1) %obj) gc "statepoint-example" {
entry:
br i1 %cmp, label %taken, label %untaken
@@ -71,10 +70,10 @@ merge: ; preds = %untaken, %taken
; CHECK-LABEL: merge:
; CHECK-NEXT: %.0 = phi ptr addrspace(1) [ %obj.relocated, %taken ], [ %obj.relocated2, %untaken ]
; CHECK-NEXT: ret ptr addrspace(1) %.0
-; When run over a function which doesn't opt in, should do nothing!
ret ptr addrspace(1) %obj
}
+; When run over a function which doesn't opt in, should do nothing!
define ptr addrspace(1) @test5(ptr addrspace(1) %obj) gc "ocaml" {
; CHECK-LABEL: @test5
entry:
diff --git a/llvm/test/Transforms/RewriteStatepointsForGC/codegen-cond.ll b/llvm/test/Transforms/RewriteStatepointsForGC/codegen-cond.ll
index be3b01d68827c..f26d6e25bbc7d 100644
--- a/llvm/test/Transforms/RewriteStatepointsForGC/codegen-cond.ll
+++ b/llvm/test/Transforms/RewriteStatepointsForGC/codegen-cond.ll
@@ -1,7 +1,6 @@
; RUN: opt -passes=rewrite-statepoints-for-gc -S < %s | FileCheck %s
; A null test of a single value
-
define i1 @test(ptr addrspace(1) %p, i1 %rare) gc "statepoint-example" {
; CHECK-LABEL: @test
entry:
@@ -19,7 +18,6 @@ continue: ; preds = %safepoint, %entry
; CHECK-DAG: [ %p, %entry ]
; CHECK: %cond = icmp
; CHECK: br i1 %cond
-; Comparing two pointers
br i1 %cond, label %taken, label %untaken
taken: ; preds = %continue
@@ -29,6 +27,7 @@ untaken: ; preds = %continue
ret i1 false
}
+; Comparing two pointers
define i1 @test2(ptr addrspace(1) %p, ptr addrspace(1) %q, i1 %rare) gc "statepoint-example" {
; CHECK-LABEL: @test2
entry:
@@ -49,8 +48,6 @@ continue: ; preds = %safepoint, %entry
; CHECK-DAG: [ %p, %entry ]
; CHECK: %cond = icmp
; CHECK: br i1 %cond
-; Check that nothing bad happens if already last instruction
-; before terminator
br i1 %cond, label %taken, label %untaken
taken: ; preds = %continue
@@ -60,6 +57,8 @@ untaken: ; preds = %continue
ret i1 false
}
+; Check that nothing bad happens if already last instruction
+; before terminator
define i1 @test3(ptr addrspace(1) %p, ptr addrspace(1) %q, i1 %rare) gc "statepoint-example" {
; CHECK-LABEL: @test3
; CHECK: gc.statepoint
diff --git a/llvm/test/Transforms/RewriteStatepointsForGC/constants.ll b/llvm/test/Transforms/RewriteStatepointsForGC/constants.ll
index d1459bc69fe91..547074e4a08d3 100644
--- a/llvm/test/Transforms/RewriteStatepointsForGC/constants.ll
+++ b/llvm/test/Transforms/RewriteStatepointsForGC/constants.ll
@@ -2,28 +2,27 @@
target datalayout = "e-ni:1:6"
-; constants don't get relocated.
@G = addrspace(1) global i8 5
declare void @foo()
+; constants don't get relocated.
define i8 @test() gc "statepoint-example" {
; CHECK-LABEL: @test
; CHECK: gc.statepoint
; CHECK-NEXT: load i8, ptr addrspace(1) inttoptr (i64 15 to ptr addrspace(1))
-; Mostly just here to show reasonable code test can come from.
entry:
call void @foo() [ "deopt"() ]
%res = load i8, ptr addrspace(1) inttoptr (i64 15 to ptr addrspace(1))
ret i8 %res
}
+; Mostly just here to show reasonable code test can come from.
define i8 @test2(ptr addrspace(1) %p) gc "statepoint-example" {
; CHECK-LABEL: @test2
; CHECK: gc.statepoint
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: icmp
-; Globals don't move and thus don't get relocated
entry:
call void @foo() [ "deopt"() ]
%cmp = icmp eq ptr addrspace(1) %p, null
@@ -37,11 +36,17 @@ not_taken: ; preds = %entry
br i1 %cmp2, label %taken, label %dead
dead: ; preds = %not_taken
+ ; We see that dead can't be reached, but the optimizer might not. It's
+ ; completely legal for it to exploit the fact that if dead executed, %p
+ ; would have to equal null. This can produce intermediate states which
+ ; look like that of test above, even if arbitrary constant addresses aren't
+ ; legal in the source language
%addr = getelementptr i8, ptr addrspace(1) %p, i32 15
%res = load i8, ptr addrspace(1) %addr
ret i8 %res
}
+; Globals don't move and thus don't get relocated
define i8 @test3(i1 %always_true) gc "statepoint-example" {
; CHECK-LABEL: @test3
; CHECK: gc.statepoint
@@ -274,4 +279,4 @@ entry:
; CHECK-DAG: call {{.*}}gc.relocate{{.*}}(%val.base, %val.base)
; CHECK-DAG: call {{.*}}gc.relocate{{.*}}(%val.base, %val)
ret ptr addrspace(1) %val
-}
\ No newline at end of file
+}
diff --git a/llvm/test/Transforms/RewriteStatepointsForGC/liveness-basics.ll b/llvm/test/Transforms/RewriteStatepointsForGC/liveness-basics.ll
index e827ff6e6e6a6..0d9dc24e06e00 100644
--- a/llvm/test/Transforms/RewriteStatepointsForGC/liveness-basics.ll
+++ b/llvm/test/Transforms/RewriteStatepointsForGC/liveness-basics.ll
@@ -4,7 +4,6 @@
; Tests to make sure we consider %obj live in both the taken and untaken
; predeccessor of merge.
-
define ptr addrspace(1) @test1(i1 %cmp, ptr addrspace(1) %obj) gc "statepoint-example" {
; CHECK-LABEL: @test1
entry:
@@ -30,10 +29,10 @@ merge: ; preds = %untaken, %taken
; CHECK-LABEL: merge:
; CHECK-NEXT: %.0 = phi ptr addrspace(1) [ %obj.relocated, %taken ], [ %obj.relocated2, %untaken ]
; CHECK-NEXT: ret ptr addrspace(1) %.0
-; A local kill should not effect liveness in predecessor block
ret ptr addrspace(1) %obj
}
+; A local kill should not effect liveness in predecessor block
define ptr addrspace(1) @test2(i1 %cmp, ptr %loc) gc "statepoint-example" {
; CHECK-LABEL: @test2
entry:
@@ -49,8 +48,6 @@ taken: ; preds = %entry
; CHECK-NEXT: gc.statepoint
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: ret ptr addrspace(1) %obj.relocated
-; A local kill should effect values live from a successor phi. Also, we
-; should only propagate liveness from a phi to the appropriate predecessors.
%obj = load ptr addrspace(1), ptr %loc
call void @foo() [ "deopt"() ]
ret ptr addrspace(1) %obj
@@ -59,6 +56,8 @@ untaken: ; preds = %entry
ret ptr addrspace(1) null
}
+; A local kill should effect values live from a successor phi. Also, we
+; should only propagate liveness from a phi to the appropriate predecessors.
define ptr addrspace(1) @test3(i1 %cmp, ptr %loc) gc "statepoint-example" {
; CHECK-LABEL: @test3
entry:
@@ -80,8 +79,6 @@ untaken: ; preds = %entry
; CHECK-LABEL: taken:
; CHECK-NEXT: gc.statepoint
; CHECK-NEXT: br label %merge
-; A base pointer must be live if it is needed at a later statepoint,
-; even if the base pointer is otherwise unused.
call void @foo() [ "deopt"() ]
br label %merge
@@ -90,6 +87,8 @@ merge: ; preds = %untaken, %taken
ret ptr addrspace(1) %phi
}
+; A base pointer must be live if it is needed at a later statepoint,
+; even if the base pointer is otherwise unused.
define ptr addrspace(1) @test4(i1 %cmp, ptr addrspace(1) %obj) gc "statepoint-example" {
; CHECK-LABEL: @test4
entry:
@@ -105,9 +104,6 @@ entry:
; CHECK-NEXT: %obj.relocated3 =
; CHECK-NEXT: ret ptr addrspace(1) %derived.relocated2
;
-; Make sure that a phi def visited during iteration is considered a kill.
-; Also, liveness after base pointer analysis can change based on new uses,
-; not just new defs.
%derived = getelementptr i64, ptr addrspace(1) %obj, i64 8
call void @foo() [ "deopt"() ]
call void @foo() [ "deopt"() ]
@@ -116,6 +112,9 @@ entry:
declare void @consume(...) readonly "gc-leaf-function"
+; Make sure that a phi def visited during iteration is considered a kill.
+; Also, liveness after base pointer analysis can change based on new uses,
+; not just new defs.
define ptr addrspace(1) @test5(i1 %cmp, ptr addrspace(1) %obj) gc "statepoint-example" {
; CHECK-LABEL: @test5
entry:
diff --git a/llvm/test/Transforms/RewriteStatepointsForGC/preprocess.ll b/llvm/test/Transforms/RewriteStatepointsForGC/preprocess.ll
index 4c7f9d66d92f6..7ac2d053821e3 100644
--- a/llvm/test/Transforms/RewriteStatepointsForGC/preprocess.ll
+++ b/llvm/test/Transforms/RewriteStatepointsForGC/preprocess.ll
@@ -1,10 +1,9 @@
; RUN: opt -passes=rewrite-statepoints-for-gc -S < %s | FileCheck %s
-; Test to make sure we destroy LCSSA's single entry phi nodes before
-; running liveness
-
declare void @consume(...) "gc-leaf-function"
+; Test to make sure we destroy LCSSA's single entry phi nodes before
+; running liveness
define void @test6(ptr addrspace(1) %obj) gc "statepoint-example" {
; CHECK-LABEL: @test6
entry:
@@ -16,7 +15,6 @@ next: ; preds = %entry
; CHECK-NEXT: gc.relocate
; CHECK-NEXT: @consume(ptr addrspace(1) %obj.relocated)
; CHECK-NEXT: @consume(ptr addrspace(1) %obj.relocated)
-; Need to delete unreachable gc.statepoint call
%obj2 = phi ptr addrspace(1) [ %obj, %entry ]
call void @foo() [ "deopt"() ]
call void (...) @consume(ptr addrspace(1) %obj2)
@@ -24,11 +22,10 @@ next: ; preds = %entry
ret void
}
+; Need to delete unreachable gc.statepoint call
define void @test7() gc "statepoint-example" {
; CHECK-LABEL: test7
; CHECK-NOT: gc.statepoint
-; Need to delete unreachable gc.statepoint invoke - tested separately given
-; a correct implementation could only remove the instructions, not the block
ret void
unreached: ; preds = %unreached
@@ -38,6 +35,8 @@ unreached: ; preds = %unreached
br label %unreached
}
+; Need to delete unreachable gc.statepoint invoke - tested separately given
+; a correct implementation could only remove the instructions, not the block
define void @test8() gc "statepoint-example" personality ptr undef {
; CHECK-LABEL: test8
; CHECK-NOT: gc.statepoint
diff --git a/llvm/test/Transforms/RewriteStatepointsForGC/relocation.ll b/llvm/test/Transforms/RewriteStatepointsForGC/relocation.ll
index 249b3c1d09ac1..be6e7a9005b04 100644
--- a/llvm/test/Transforms/RewriteStatepointsForGC/relocation.ll
+++ b/llvm/test/Transforms/RewriteStatepointsForGC/relocation.ll
@@ -88,11 +88,11 @@ else_branch: ; preds = %bci_0
; CHECK-LABEL: else_branch:
; CHECK: gc.statepoint
; CHECK: gc.relocate
-; We need to end up with a single relocation phi updated from both paths
call void @foo() [ "deopt"() ]
br label %join
join: ; preds = %else_branch, %if_branch
+; We need to end up with a single relocation phi updated from both paths
; CHECK-LABEL: join:
; CHECK: phi ptr addrspace(1)
; CHECK-DAG: [ %arg.relocated, %if_branch ]
@@ -246,11 +246,11 @@ inner-loop: ; preds = %inner-loop, %outer-
outer-inc: ; preds = %inner-loop
; CHECK-LABEL: outer-inc:
-; This test shows why updating just those uses of the original value being
-; relocated dominated by the inserted relocation is not always sufficient.
br label %outer-loop
}
+; This test shows why updating just those uses of the original value being
+; relocated dominated by the inserted relocation is not always sufficient.
define ptr addrspace(1) @test7(ptr addrspace(1) %obj, ptr addrspace(1) %obj2, i1 %condition) gc "statepoint-example" {
; CHECK-LABEL: @test7
entry:
@@ -269,6 +269,9 @@ join: ; preds = %callbb, %entry
; CHECK: phi ptr addrspace(1)
; CHECK-DAG: [ %obj, %entry ]
; CHECK-DAG: [ %obj2.relocated, %callbb ]
+ ; This is a phi outside the dominator region of the new defs inserted by
+ ; the safepoint, BUT we can't stop the search here or we miss the second
+ ; phi below.
%phi1 = phi ptr addrspace(1) [ %obj, %entry ], [ %obj2, %callbb ]
br label %join2
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The commit 0407108 did some transformations on tests for RewriteStatepointsForGC that put some comments out-of-order. At least the
basics.lltest is referred to by the current LLVM statepoints documentation, so this may have caused confusion to end-users investigating further into statepoint usage.Additionally, some comments were outright removed that explained potentially non-trivial behaviour of the pass in the tests.
This is my first time ever submitting a patch to LLVM, so please let me know if there's anything I accidentally overlooked. I re-tested all tests to ensure no issues accidentally arose from these changes.