llvm-project icon indicating copy to clipboard operation
llvm-project copied to clipboard

[AArch64] Mark neon.stN intrinsics as writeonly

Open nikic opened this issue 6 months ago • 3 comments

I found this peculiar comment in EarlyCSE: https://github.com/llvm/llvm-project/blob/1c78d8d9d7bcb4b20910047ad7db35f177a17c8c/llvm/lib/Transforms/Scalar/EarlyCSE.cpp#L1620-L1624

Looking back over history, this seems to be referring to the aarch64.neon.stN intrinsics, which are indeed not marked writeonly (though the ldN intrinsics are readonly).

Possibly I'm missing something special about these intrinsics, but I think it is safe to mark them as writeonly.

nikic avatar Jun 23 '25 08:06 nikic

@llvm/pr-subscribers-backend-arm @llvm/pr-subscribers-llvm-analysis @llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-backend-aarch64

Author: Nikita Popov (nikic)

Changes

I found this peculiar comment in EarlyCSE: https://github.com/llvm/llvm-project/blob/1c78d8d9d7bcb4b20910047ad7db35f177a17c8c/llvm/lib/Transforms/Scalar/EarlyCSE.cpp#L1620-L1624

Looking back over history, this seems to be referring to the aarch64.neon.stN intrinsics, which are indeed not marked writeonly (though the ldN intrinsics are readonly).

Possibly I'm missing something special about these intrinsics, but I think it is safe to mark them as writeonly.


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

1 Files Affected:

  • (modified) llvm/include/llvm/IR/IntrinsicsAArch64.td (+7-7)
diff --git a/llvm/include/llvm/IR/IntrinsicsAArch64.td b/llvm/include/llvm/IR/IntrinsicsAArch64.td
index 0ec5f5163118e..3606bbe29eb93 100644
--- a/llvm/include/llvm/IR/IntrinsicsAArch64.td
+++ b/llvm/include/llvm/IR/IntrinsicsAArch64.td
@@ -612,7 +612,7 @@ let TargetPrefix = "aarch64" in {  // All intrinsics start with "llvm.aarch64.".
                   [IntrReadMem, IntrArgMemOnly]>;
   class AdvSIMD_1Vec_Store_Lane_Intrinsic
     : DefaultAttrsIntrinsic<[], [llvm_anyvector_ty, llvm_i64_ty, llvm_anyptr_ty],
-                [IntrArgMemOnly, NoCapture<ArgIndex<2>>]>;
+                [IntrWriteMem, IntrArgMemOnly, NoCapture<ArgIndex<2>>]>;
 
   class AdvSIMD_2Vec_Load_Intrinsic
     : DefaultAttrsIntrinsic<[LLVMMatchType<0>, llvm_anyvector_ty],
@@ -626,11 +626,11 @@ let TargetPrefix = "aarch64" in {  // All intrinsics start with "llvm.aarch64.".
   class AdvSIMD_2Vec_Store_Intrinsic
     : DefaultAttrsIntrinsic<[], [llvm_anyvector_ty, LLVMMatchType<0>,
                      llvm_anyptr_ty],
-                [IntrArgMemOnly, NoCapture<ArgIndex<2>>]>;
+                [IntrWriteMem, IntrArgMemOnly, NoCapture<ArgIndex<2>>]>;
   class AdvSIMD_2Vec_Store_Lane_Intrinsic
     : DefaultAttrsIntrinsic<[], [llvm_anyvector_ty, LLVMMatchType<0>,
                  llvm_i64_ty, llvm_anyptr_ty],
-                [IntrArgMemOnly, NoCapture<ArgIndex<3>>]>;
+                [IntrWriteMem, IntrArgMemOnly, NoCapture<ArgIndex<3>>]>;
 
   class AdvSIMD_3Vec_Load_Intrinsic
     : DefaultAttrsIntrinsic<[LLVMMatchType<0>, LLVMMatchType<0>, llvm_anyvector_ty],
@@ -644,12 +644,12 @@ let TargetPrefix = "aarch64" in {  // All intrinsics start with "llvm.aarch64.".
   class AdvSIMD_3Vec_Store_Intrinsic
     : DefaultAttrsIntrinsic<[], [llvm_anyvector_ty, LLVMMatchType<0>,
                      LLVMMatchType<0>, llvm_anyptr_ty],
-                [IntrArgMemOnly, NoCapture<ArgIndex<3>>]>;
+                [IntrWriteMem, IntrArgMemOnly, NoCapture<ArgIndex<3>>]>;
   class AdvSIMD_3Vec_Store_Lane_Intrinsic
     : DefaultAttrsIntrinsic<[], [llvm_anyvector_ty,
                  LLVMMatchType<0>, LLVMMatchType<0>,
                  llvm_i64_ty, llvm_anyptr_ty],
-                [IntrArgMemOnly, NoCapture<ArgIndex<4>>]>;
+                [IntrWriteMem, IntrArgMemOnly, NoCapture<ArgIndex<4>>]>;
 
   class AdvSIMD_4Vec_Load_Intrinsic
     : DefaultAttrsIntrinsic<[LLVMMatchType<0>, LLVMMatchType<0>,
@@ -667,12 +667,12 @@ let TargetPrefix = "aarch64" in {  // All intrinsics start with "llvm.aarch64.".
     : DefaultAttrsIntrinsic<[], [llvm_anyvector_ty, LLVMMatchType<0>,
                  LLVMMatchType<0>, LLVMMatchType<0>,
                  llvm_anyptr_ty],
-                [IntrArgMemOnly, NoCapture<ArgIndex<4>>]>;
+                [IntrWriteMem, IntrArgMemOnly, NoCapture<ArgIndex<4>>]>;
   class AdvSIMD_4Vec_Store_Lane_Intrinsic
     : DefaultAttrsIntrinsic<[], [llvm_anyvector_ty, LLVMMatchType<0>,
                  LLVMMatchType<0>, LLVMMatchType<0>,
                  llvm_i64_ty, llvm_anyptr_ty],
-                [IntrArgMemOnly, NoCapture<ArgIndex<5>>]>;
+                [IntrWriteMem, IntrArgMemOnly, NoCapture<ArgIndex<5>>]>;
 }
 
 // Memory ops

llvmbot avatar Jun 23 '25 08:06 llvmbot

I don't know of a reason why this would be a problem. The MVE intrinsics already specify IntrWriteMem. Should we add it to the similar Arm neon intrinsics too, either here or in a followup?

davemgreen avatar Jun 23 '25 15:06 davemgreen

I don't know of a reason why this would be a problem. The MVE intrinsics already specify IntrWriteMem. Should we add it to the similar Arm neon intrinsics too, either here or in a followup?

I've added the Arm variants now as well.

nikic avatar Jun 30 '25 10:06 nikic