Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CodeGen][GC] Skip function without GC in GCLoweringPass #84421

Merged
merged 4 commits into from
Mar 14, 2024

Conversation

paperchalice
Copy link
Contributor

No description provided.

@aeubanks
Copy link
Contributor

aeubanks commented Mar 8, 2024

is this purely a compile time optimization? or does this actually have functional change (in which case should have a test)

@paperchalice
Copy link
Contributor Author

is this purely a compile time optimization? or does this actually have functional change (in which case should have a test)

It is a pass to lower GC intrinsics, GC related passes are lack of testing.

@llvmbot
Copy link
Collaborator

llvmbot commented Mar 8, 2024

@llvm/pr-subscribers-backend-x86

Author: None (paperchalice)

Changes

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

2 Files Affected:

  • (modified) llvm/lib/CodeGen/GCRootLowering.cpp (+3)
  • (modified) llvm/test/CodeGen/X86/GC/alloc_loop.ll (+7)
diff --git a/llvm/lib/CodeGen/GCRootLowering.cpp b/llvm/lib/CodeGen/GCRootLowering.cpp
index 894ab9a0486a7b..0d82f2bab8960b 100644
--- a/llvm/lib/CodeGen/GCRootLowering.cpp
+++ b/llvm/lib/CodeGen/GCRootLowering.cpp
@@ -81,6 +81,9 @@ class GCMachineCodeAnalysis : public MachineFunctionPass {
 
 PreservedAnalyses GCLoweringPass::run(Function &F,
                                       FunctionAnalysisManager &FAM) {
+  if (!F.hasGC())
+    return PreservedAnalyses::all();
+
   auto &Info = FAM.getResult<GCFunctionAnalysis>(F);
 
   bool Changed = DoLowering(F, Info.getStrategy());
diff --git a/llvm/test/CodeGen/X86/GC/alloc_loop.ll b/llvm/test/CodeGen/X86/GC/alloc_loop.ll
index 4b7230fee9c5a9..4e87735e830ffe 100644
--- a/llvm/test/CodeGen/X86/GC/alloc_loop.ll
+++ b/llvm/test/CodeGen/X86/GC/alloc_loop.ll
@@ -1,4 +1,5 @@
 ; RUN: llc -mtriple=x86_64 < %s
+; RUN: opt -S -passes='require<collector-metadata>,function(gc-lowering)' < %s | FileCheck %s
 
 declare ptr @llvm_gc_allocate(i32)
 declare void @llvm_gc_initialize(i32)
@@ -31,6 +32,8 @@ entry:
 	%B.1 = load ptr, ptr %B
 	%A.1 = load ptr, ptr %A
 	call void @llvm.gcwrite(ptr %A.1, ptr %B.upgrd.1, ptr %B.1)
+	; CHECK-NOT: call void @llvm.gcwrite(ptr %A.1, ptr %B.upgrd.1, ptr %B.1)
+	; CHECK: store ptr %A.1, ptr %B.1, align 8
 	
 	br label %AllocLoop
 
@@ -47,4 +50,8 @@ Exit:
 	ret i32 0
 }
 
+define void @no_gc() {
+	ret void
+}
+
 declare void @__main()

@paperchalice
Copy link
Contributor Author

Find a place to add related test.

@@ -31,6 +32,8 @@ entry:
%B.1 = load ptr, ptr %B
%A.1 = load ptr, ptr %A
call void @llvm.gcwrite(ptr %A.1, ptr %B.upgrd.1, ptr %B.1)
; CHECK-NOT: call void @llvm.gcwrite(ptr %A.1, ptr %B.upgrd.1, ptr %B.1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-NOT checks are perilous and should be as lax as possible. Better would be -NEXT checks to show it's not present

@paperchalice
Copy link
Contributor Author

Move test to CodeGen/Generic, and use CHECK-NEXT.


define i32 @main() gc "shadow-stack" {
entry:
%A = alloca ptr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test whitespace is weird

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, mix tab and space.

Copy link
Contributor

@aeubanks aeubanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use llvm/utils/update_test_checks.py for the test?

@paperchalice paperchalice merged commit edc2066 into llvm:main Mar 14, 2024
4 checks passed
@paperchalice paperchalice deleted the skip-gc branch March 14, 2024 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants