Skip to content

[IR] Allow llvm.experimental.memset.pattern to take any sized type as the pattern argument #132026

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

Merged
merged 2 commits into from
Mar 19, 2025

Conversation

asb
Copy link
Contributor

@asb asb commented Mar 19, 2025

I initially thought starting with a more narrow definition and later expanding would make more sense. But as pointed out in review for PR #129220, this restriction is generating additional unnecessary work.

This patch alters the intrinsic to accept patterns of any type. Future patches will update LoopIdiomRecognize and PreISelIntrinsicLowering to take advantage of this. The verifier will complain if an unsized type is used. I've additionally taken the opportunity to remove a comment from the LangRef about some bit widths potentially not being supported by the target. I don't think this is any more true than it is for arbitrary width loads/stores which don't carry a similar warning that I can see.

A verifier check ensures that only sized types are used for the pattern (unless I'm missing it, I don't believe I can represent that restriction in Intrinsics.td).

… the pattern argument

I initially thought starting with a more narrow definition and later
expanding would make more sense. But as pointed out in review for
PR llvm#129220, this restriction is generating additional unnecessary work.

This patch alters the intrinsic to accept patterns of any type. Future
patches will update LoopIdiomRecognize and PreISelIntrinsicLowering to
take advantage of this. The verifier will complain if an unsized type is
used. I've additionally taken the opportunity to remove a comment from
the LangRef about some bit widths potentially not being supported by the
target. I don't think this is any more true than it is for arbitrary
width loads/stores which don't carry a similar warning that I can see.
@asb asb requested review from arsenm, nikic and preames March 19, 2025 13:41
@llvmbot
Copy link
Member

llvmbot commented Mar 19, 2025

@llvm/pr-subscribers-llvm-ir

Author: Alex Bradbury (asb)

Changes

I initially thought starting with a more narrow definition and later expanding would make more sense. But as pointed out in review for PR #129220, this restriction is generating additional unnecessary work.

This patch alters the intrinsic to accept patterns of any type. Future patches will update LoopIdiomRecognize and PreISelIntrinsicLowering to take advantage of this. The verifier will complain if an unsized type is used. I've additionally taken the opportunity to remove a comment from the LangRef about some bit widths potentially not being supported by the target. I don't think this is any more true than it is for arbitrary width loads/stores which don't carry a similar warning that I can see.

A verifier check ensures that only sized types are used for the pattern (unless I'm missing it, I don't believe I can represent that restriction in Intrinsics.td).


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

4 Files Affected:

  • (modified) llvm/docs/LangRef.rst (+2-2)
  • (modified) llvm/include/llvm/IR/Intrinsics.td (+1-1)
  • (modified) llvm/lib/IR/Verifier.cpp (+4)
  • (added) llvm/test/Verifier/memset-pattern-unsized.ll (+10)
diff --git a/llvm/docs/LangRef.rst b/llvm/docs/LangRef.rst
index bda80d76dfaa5..844160822ccef 100644
--- a/llvm/docs/LangRef.rst
+++ b/llvm/docs/LangRef.rst
@@ -15640,8 +15640,8 @@ Syntax:
 """""""
 
 This is an overloaded intrinsic. You can use
-``llvm.experimental.memset.pattern`` on any integer bit width and for
-different address spaces. Not all targets support all bit widths however.
+``llvm.experimental.memset.pattern`` on any sized type for different address
+spaces.
 
 ::
 
diff --git a/llvm/include/llvm/IR/Intrinsics.td b/llvm/include/llvm/IR/Intrinsics.td
index 562e0714d879a..d10b07ccd91c2 100644
--- a/llvm/include/llvm/IR/Intrinsics.td
+++ b/llvm/include/llvm/IR/Intrinsics.td
@@ -1025,7 +1025,7 @@ def int_memset_inline
 def int_experimental_memset_pattern
     : Intrinsic<[],
       [llvm_anyptr_ty, // Destination.
-       llvm_anyint_ty, // Pattern value.
+       llvm_any_ty,    // Pattern value.
        llvm_anyint_ty, // Count (number of times to fill value).
        llvm_i1_ty],    // IsVolatile.
       [IntrWriteMem, IntrArgMemOnly, IntrWillReturn, IntrNoFree, IntrNoCallback,
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 524e9647bfd35..5934f7adffb93 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -5583,7 +5583,11 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
   case Intrinsic::memmove:
   case Intrinsic::memset:
   case Intrinsic::memset_inline:
+    break;
   case Intrinsic::experimental_memset_pattern: {
+    const auto Memset = cast<MemSetPatternInst>(&Call);
+    Check(Memset->getValue()->getType()->isSized(),
+          "unsized types cannot be used as memset patterns", Call);
     break;
   }
   case Intrinsic::memcpy_element_unordered_atomic:
diff --git a/llvm/test/Verifier/memset-pattern-unsized.ll b/llvm/test/Verifier/memset-pattern-unsized.ll
new file mode 100644
index 0000000000000..71b7dca9a5a19
--- /dev/null
+++ b/llvm/test/Verifier/memset-pattern-unsized.ll
@@ -0,0 +1,10 @@
+; RUN: not opt -passes=verify < %s 2>&1 | FileCheck %s
+
+; CHECK: unsized types cannot be used as memset patterns
+
+%X = type opaque
+define void @bar(ptr %P, %X %value) {
+  call void @llvm.experimental.memset.pattern.p0.s_s.i32.0(ptr %P, %X %value, i32 4, i1 false)
+  ret void
+}
+declare void @llvm.experimental.memset.pattern.p0.s_s.i32.0(ptr nocapture, %X, i32, i1) nounwind

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, thanks!

@asb asb merged commit be0a3b2 into llvm:main Mar 19, 2025
7 of 10 checks passed
asb added a commit that referenced this pull request Mar 27, 2025
…memset.pattern with ptr pattern argument

These tests all show the desired output because after #132026, the
intrinsic can take any sized type as an argument. The tests were adapted
from those I proposed adding in #129220.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants