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

[SROA] Allow llvm.launder.invariant.group intrinsic to be splittable #72056

Conversation

antoniofrighetto
Copy link
Contributor

Fixes: #72035.

@llvmbot
Copy link
Collaborator

llvmbot commented Nov 12, 2023

@llvm/pr-subscribers-llvm-transforms

Author: Antonio Frighetto (antoniofrighetto)

Changes

Fixes: #72035.


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

2 Files Affected:

  • (modified) llvm/lib/Transforms/Scalar/SROA.cpp (+6-1)
  • (modified) llvm/test/Transforms/SROA/invariant-group.ll (+30)
diff --git a/llvm/lib/Transforms/Scalar/SROA.cpp b/llvm/lib/Transforms/Scalar/SROA.cpp
index 321525d3741d0a2..095f12cd414b803 100644
--- a/llvm/lib/Transforms/Scalar/SROA.cpp
+++ b/llvm/lib/Transforms/Scalar/SROA.cpp
@@ -1144,6 +1144,7 @@ class AllocaSlices::SliceBuilder : public PtrUseVisitor<SliceBuilder> {
     }
 
     if (II.isLaunderOrStripInvariantGroup()) {
+      insertUse(II, Offset, AllocSize, true);
       enqueueUsers(II);
       return;
     }
@@ -3328,7 +3329,8 @@ class llvm::sroa::AllocaSliceRewriter
   }
 
   bool visitIntrinsicInst(IntrinsicInst &II) {
-    assert((II.isLifetimeStartOrEnd() || II.isDroppable()) &&
+    assert((II.isLifetimeStartOrEnd() || II.isLaunderOrStripInvariantGroup() ||
+            II.isDroppable()) &&
            "Unexpected intrinsic!");
     LLVM_DEBUG(dbgs() << "    original: " << II << "\n");
 
@@ -3342,6 +3344,9 @@ class llvm::sroa::AllocaSliceRewriter
       return true;
     }
 
+    if (II.isLaunderOrStripInvariantGroup())
+      return true;
+
     assert(II.getArgOperand(1) == OldPtr);
     // Lifetime intrinsics are only promotable if they cover the whole alloca.
     // Therefore, we drop lifetime intrinsics which don't cover the whole
diff --git a/llvm/test/Transforms/SROA/invariant-group.ll b/llvm/test/Transforms/SROA/invariant-group.ll
index 083fa72026b85e6..eceeca2c997bab9 100644
--- a/llvm/test/Transforms/SROA/invariant-group.ll
+++ b/llvm/test/Transforms/SROA/invariant-group.ll
@@ -79,6 +79,36 @@ define void @g() {
   ret void
 }
 
+define void @store_and_launder() {
+; CHECK-LABEL: @store_and_launder(
+; CHECK-NEXT:    ret void
+;
+  %valptr = alloca i32, align 4
+  store i32 0, ptr %valptr, align 4
+  %barr = call ptr @llvm.launder.invariant.group.p0(ptr %valptr)
+  ret void
+}
+
+define i32 @launder_and_load() {
+; CHECK-LABEL: @launder_and_load(
+; CHECK-NEXT:    ret i32 undef
+;
+  %valptr = alloca i32, align 4
+  %barr = call ptr @llvm.launder.invariant.group.p0(ptr %valptr)
+  %v2 = load i32, ptr %valptr
+  ret i32 %v2
+}
+
+define void @launder_and_ptr_arith() {
+; CHECK-LABEL: @launder_and_ptr_arith(
+; CHECK-NEXT:    ret void
+;
+  %valptr = alloca i32, align 4
+  %barr = call ptr @llvm.launder.invariant.group.p0(ptr %valptr)
+  %a2 = getelementptr inbounds i32, ptr %valptr, i32 0
+  ret void
+}
+
 !0 = !{}
 ;; NOTE: These prefixes are unused and the list is autogenerated. Do not add tests below this line:
 ; CHECK-MODIFY-CFG: {{.*}}

@nikic
Copy link
Contributor

nikic commented Nov 12, 2023

Can you please test a case where part of the alloca needs to be retained?

@antoniofrighetto antoniofrighetto force-pushed the feature/sroa-improve-rewrite-launder-intrinsic branch from e2be56a to 5073a5a Compare November 12, 2023 16:40
@antoniofrighetto
Copy link
Contributor Author

Not exactly sure if the optimized IR of partial_use_of_struct test is what we want.

@nikic
Copy link
Contributor

nikic commented Nov 12, 2023

Not exactly sure if the optimized IR of partial_use_of_struct test is what we want.

That's not it. I'm looking for a case where the alloca is split and some parts are promoted, but others aren't, e.g. because they are accessed via volatile operations. There should still be an alloca left at the end, and the launder.invariant.group needs to be rewritten (and possibly duplicated) appropriately.

@antoniofrighetto antoniofrighetto force-pushed the feature/sroa-improve-rewrite-launder-intrinsic branch 2 times, most recently from 947eda0 to 1dba1f9 Compare November 13, 2023 21:28
@antoniofrighetto
Copy link
Contributor Author

I think it should look better now.

ret void
}

define void @partial_promotion_of_alloca() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please also add some invariant group metadata to this test? It looks like the launder.invariant intrinsic gets dropped, which is okay only if the metadata also gets dropped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, thanks! AFAIU, in our case, the invariant.group should be added to memory accesses to the same pointer operand.

@antoniofrighetto antoniofrighetto force-pushed the feature/sroa-improve-rewrite-launder-intrinsic branch from 1dba1f9 to 9ac0bbe Compare November 16, 2023 10:16
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

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

LGTM

br i1 %cond, label %use_alloca, label %end

use_alloca:
call void @use(i32* nonnull %valptr)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
call void @use(i32* nonnull %valptr)
call void @use(ptr nonnull %valptr)

Let `llvm.launder.invariant.group` intrinsic as well as instructions
operating on memory addresses, whose invariance may be broken by the
intrinsic, to be rewritten.

Fixes: llvm#72035.
@antoniofrighetto antoniofrighetto force-pushed the feature/sroa-improve-rewrite-launder-intrinsic branch from 9ac0bbe to ebbb9cd Compare November 16, 2023 16:56
@antoniofrighetto antoniofrighetto merged commit ebbb9cd into llvm:main Nov 16, 2023
1 of 3 checks passed
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.

SROA crash
3 participants