Add IntrOnlyWrite intrinsic property

[nhaehnle]

Summary

This property is used to mark an intrinsic that writes to memory, but is
lowered to instructions without the mayLoad or hasSideEffects flag because the
target-specific code understands its memory behavior fully.

An example for this is the llvm.amdgcn.buffer.store.format.* intrinsic, which
corresponds to a store instruction that goes through a special buffer
descriptor rather than through a plain pointer.

With this property, the intrinsic should still be handled as having side
effects at the LLVM IR level, but machine scheduling can make smarter
decisions.

[nhaehnle]

Reorganize the code a bit, taking into account some feedback via llvm-dev
and elsewhere.

This patch only adds the LLVM intrinsic property to keep it simple and limited
to the AMDGPU use case.

[mehdi_amini]

Looks OK in principle, but please see inline comments.

utils/TableGen/CodeGenIntrinsics.h

67 ↗

(On Diff #52424)

I found it confusing that these are used to define the enum value below and also separately in the code. Maybe use a separate enum?
Also documenting the lattice would be nice.
The individual value meaning might be worth a comment, for instance MR_Read is *read through argument only* from what I understand below (and the name does not convey this).

utils/TableGen/IntrinsicEmitter.cpp

603 ↗

(On Diff #52424)

Don't you get a warning for fully covered switch?

[nhaehnle]

Changes to the ModRefBehavior enum in TableGen.

[nhaehnle]

utils/TableGen/CodeGenIntrinsics.h

67 ↗

(On Diff #52424)

The intention is to be analogous to the FunctionModRefBehavior in AliasAnalysis. The values of the bits are different to preserve the ordering of the possible numerical values.

I'm changing MR_Read/Write to MR_Ref/Mod to make this clearer, and putting the bits into their own enum.

utils/TableGen/IntrinsicEmitter.cpp

603 ↗

(On Diff #52424)

Not originally, because of the non-sensical possibility of MR_Anywhere without MR_Read/Ref or MR_Write/Mod. With the new enum organization it can indeed be removed. (I didn't get the warning even then, perhaps that's a gcc vs. clang thing.)

[mehdi_amini]

No other comments on my side, let's wait for Philip's review.

[nhaehnle]

Ping?

[nhaehnle]

Ping^2?

[reames]

LGTM w/comments addressed.

include/llvm/IR/Intrinsics.td

46 ↗

(On Diff #52436)

A better approach would be to split the existing ReadArgMem into an ArgMemOnly attribute and the existing ReadMemOnly attribute. I will not require that for this change, but it would be a worthwhile cleanup.

48 ↗

(On Diff #52436)

The writes can not be volatile. We model a volatile write as both reading and writing from the address. Changing that would be a much larger change.

llvm/trunk/include/llvm/IR/Intrinsics.td

 // pointer-typed arguments points to, but may read an unspecified amount.
 def IntrReadArgMem : IntrinsicProperty;
 
 // IntrReadMem - This intrinsic reads from unspecified memory, so it cannot be
 // moved across stores.  However, it can be reordered otherwise and can be
 // deleted if dead.
 def IntrReadMem : IntrinsicProperty;
 
+// IntrWriteMem - This intrinsic writes to unspecified memory, but does not
+// read from memory, and has no other side effects. This means dead stores
+// before calls to this intrinsics may be removed.
+def IntrWriteMem : IntrinsicProperty;
+
+// IntrWriteArgMem - This intrinsic writes only to memory that one of its
+// arguments points to, but may access an unspecified amount. The intrinsic
+// does not read from memory and has no other side effects. This means that
+// dead stores before calls to this intrinsics may be removed.
+def IntrWriteArgMem : IntrinsicProperty;
+
 // IntrReadWriteArgMem - This intrinsic reads and writes only from memory that
 // one of its arguments points to, but may access an unspecified amount.  The
 // reads and writes may be volatile, but except for this it has no other side
 // effects.
 def IntrReadWriteArgMem : IntrinsicProperty;
 
 // Commutative - This intrinsic is commutative: X op Y == Y op X.
 def Commutative : IntrinsicProperty;

llvm/trunk/include/llvm/IR/IntrinsicsAMDGPU.td

 class AMDGPUBufferStore : Intrinsic <
   [],
   [llvm_anyfloat_ty,  // vdata(VGPR) -- can currently only select f32, v2f32, v4f32
    llvm_v4i32_ty,     // rsrc(SGPR)
    llvm_i32_ty,       // vindex(VGPR)
    llvm_i32_ty,       // offset(SGPR/VGPR/imm)
    llvm_i1_ty,        // glc(imm)
    llvm_i1_ty],       // slc(imm)
-  []>;
+  [IntrWriteMem]>;
 def int_amdgcn_buffer_store_format : AMDGPUBufferStore;
 def int_amdgcn_buffer_store : AMDGPUBufferStore;
 
 class AMDGPUBufferAtomic : Intrinsic <
   [llvm_i32_ty],
   [llvm_i32_ty,       // vdata(VGPR)
    llvm_v4i32_ty,     // rsrc(SGPR)
    llvm_i32_ty,       // vindex(VGPR)

llvm/trunk/lib/Target/AMDGPU/SIInstructions.td

defm BUFFER_LOAD_FORMAT_XY : MUBUF_Load_Helper <
   mubuf<0x01>, "buffer_load_format_xy", VReg_64
 >;
 defm BUFFER_LOAD_FORMAT_XYZ : MUBUF_Load_Helper <
   mubuf<0x02>, "buffer_load_format_xyz", VReg_96
 >;
 defm BUFFER_LOAD_FORMAT_XYZW : MUBUF_Load_Helper <
   mubuf<0x03>, "buffer_load_format_xyzw", VReg_128
 >;
-// Without mayLoad and hasSideEffects, TableGen complains about the pattern
-// matching llvm.amdgcn.buffer.store.format. Eventually, we'll need a way
-// to express the effects of the intrinsic more precisely.
-let mayLoad = 1, hasSideEffects = 1 in {
-  defm BUFFER_STORE_FORMAT_X : MUBUF_Store_Helper <
-    mubuf<0x04>, "buffer_store_format_x", VGPR_32
-  >;
-  defm BUFFER_STORE_FORMAT_XY : MUBUF_Store_Helper <
-    mubuf<0x05>, "buffer_store_format_xy", VReg_64
-  >;
-  defm BUFFER_STORE_FORMAT_XYZ : MUBUF_Store_Helper <
-    mubuf<0x06>, "buffer_store_format_xyz", VReg_96
-  >;
-  defm BUFFER_STORE_FORMAT_XYZW : MUBUF_Store_Helper <
-    mubuf<0x07>, "buffer_store_format_xyzw", VReg_128
-  >;
-}
 defm BUFFER_LOAD_UBYTE : MUBUF_Load_Helper <
   mubuf<0x08, 0x10>, "buffer_load_ubyte", VGPR_32, i32, az_extloadi8_global
 >;
 defm BUFFER_LOAD_SBYTE : MUBUF_Load_Helper <
   mubuf<0x09, 0x11>, "buffer_load_sbyte", VGPR_32, i32, sextloadi8_global
 >;
 defm BUFFER_LOAD_USHORT : MUBUF_Load_Helper <
   mubuf<0x0a, 0x12>, "buffer_load_ushort", VGPR_32, i32, az_extloadi16_global

 defm BUFFER_STORE_BYTE : MUBUF_Store_Helper <
   mubuf<0x18>, "buffer_store_byte", VGPR_32, i32, truncstorei8_global
 >;
 
 defm BUFFER_STORE_SHORT : MUBUF_Store_Helper <
   mubuf<0x1a>, "buffer_store_short", VGPR_32, i32, truncstorei16_global
 >;
 
-// Without mayLoad and hasSideEffects, TableGen complains about the pattern
-// matching llvm.amdgcn.buffer.store. Eventually, we'll want a WriteOnly
-// property to express the effects of this intrinsic more precisely, see
-// http://reviews.llvm.org/D18291
-let mayLoad = 1, hasSideEffects = 1 in {
 defm BUFFER_STORE_DWORD : MUBUF_Store_Helper <
   mubuf<0x1c>, "buffer_store_dword", VGPR_32, i32, global_store
 >;
 
 defm BUFFER_STORE_DWORDX2 : MUBUF_Store_Helper <
   mubuf<0x1d>, "buffer_store_dwordx2", VReg_64, v2i32, global_store
 >;
 
 defm BUFFER_STORE_DWORDX4 : MUBUF_Store_Helper <
   mubuf<0x1e, 0x1f>, "buffer_store_dwordx4", VReg_128, v4i32, global_store
 >;
-}
 
 defm BUFFER_ATOMIC_SWAP : MUBUF_Atomic <
   mubuf<0x30, 0x40>, "buffer_atomic_swap", VGPR_32, i32, atomic_swap_global
 >;
 defm BUFFER_ATOMIC_CMPSWAP : MUBUF_Atomic <
   mubuf<0x31, 0x41>, "buffer_atomic_cmpswap", VReg_64, v2i32, null_frag
 >;
 defm BUFFER_ATOMIC_ADD : MUBUF_Atomic <

llvm/trunk/test/CodeGen/AMDGPU/captured-frame-index.ll

define void @stored_fi_to_lds(float* addrspace(3)* %ptr) #0 {
   %tmp = alloca float
   store float 4.0, float *%tmp
   store float* %tmp, float* addrspace(3)* %ptr
   ret void
 }
 
 ; Offset is applied
 ; GCN-LABEL: {{^}}stored_fi_to_lds_2_small_objects:
-; GCN: v_mov_b32_e32 [[ZERO:v[0-9]+]], 0{{$}}
-; GCN: buffer_store_dword v{{[0-9]+}}, [[ZERO]], s{{\[[0-9]+:[0-9]+\]}}, s{{[0-9]+}} offen{{$}}
-; GCN: buffer_store_dword v{{[0-9]+}}, [[ZERO]], s{{\[[0-9]+:[0-9]+\]}}, s{{[0-9]+}} offen offset:4{{$}}
+; GCN-DAG: v_mov_b32_e32 [[ZERO:v[0-9]+]], 0{{$}}
+; GCN-DAG: buffer_store_dword v{{[0-9]+}}, [[ZERO]], s{{\[[0-9]+:[0-9]+\]}}, s{{[0-9]+}} offen{{$}}
+; GCN-DAG: buffer_store_dword v{{[0-9]+}}, [[ZERO]], s{{\[[0-9]+:[0-9]+\]}}, s{{[0-9]+}} offen offset:4{{$}}
 
-; GCN: s_load_dword [[LDSPTR:s[0-9]+]]
+; GCN-DAG: s_load_dword [[LDSPTR:s[0-9]+]]
 
 ; GCN-DAG: v_mov_b32_e32 [[VLDSPTR:v[0-9]+]], [[LDSPTR]]
 ; GCN: ds_write_b32  [[VLDSPTR]], [[ZERO]]
 
 ; GCN-DAG: v_mov_b32_e32 [[FI1:v[0-9]+]], 4{{$}}
 ; GCN: ds_write_b32  [[VLDSPTR]], [[FI1]]
 define void @stored_fi_to_lds_2_small_objects(float* addrspace(3)* %ptr) #0 {
   %tmp0 = alloca float

llvm/trunk/test/CodeGen/AMDGPU/si-triv-disjoint-mem-access.ll

 ; RUN: llc -march=amdgcn -mcpu=bonaire -verify-machineinstrs -enable-misched -enable-aa-sched-mi < %s | FileCheck -check-prefix=FUNC -check-prefix=CI %s
 
 declare void @llvm.SI.tbuffer.store.i32(<16 x i8>, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32)
 declare void @llvm.SI.tbuffer.store.v4i32(<16 x i8>, <4 x i32>, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32)
 declare void @llvm.amdgcn.s.barrier() #1
 
 
 @stored_lds_ptr = addrspace(3) global i32 addrspace(3)* undef, align 4
 @stored_constant_ptr = addrspace(3) global i32 addrspace(2)* undef, align 8
 @stored_global_ptr = addrspace(3) global i32 addrspace(1)* undef, align 8
 
 ; FUNC-LABEL: @reorder_local_load_global_store_local_load
 ; CI: ds_read_b32 {{v[0-9]+}}, {{v[0-9]+}} offset:4
-; CI-NEXT: buffer_store_dword
 ; CI-NEXT: ds_read_b32 {{v[0-9]+}}, {{v[0-9]+}} offset:8
+; CI: buffer_store_dword
 define void @reorder_local_load_global_store_local_load(i32 addrspace(1)* %out, i32 addrspace(1)* %gptr) #0 {
   %ptr0 = load i32 addrspace(3)*, i32 addrspace(3)* addrspace(3)* @stored_lds_ptr, align 4
 
   %ptr1 = getelementptr inbounds i32, i32 addrspace(3)* %ptr0, i32 1
   %ptr2 = getelementptr inbounds i32, i32 addrspace(3)* %ptr0, i32 2
 
   %tmp1 = load i32, i32 addrspace(3)* %ptr1, align 4
   store i32 99, i32 addrspace(1)* %gptr, align 4
define void @no_reorder_barrier_local_load_global_store_local_load(i32 addrspace(1)* %out, i32 addrspace(1)* %gptr) #0 {
 
   %add = add nsw i32 %tmp1, %tmp2
 
   store i32 %add, i32 addrspace(1)* %out, align 4
   ret void
 }
 
 ; FUNC-LABEL: @reorder_constant_load_global_store_constant_load
+; CI: buffer_store_dword
 ; CI: v_readfirstlane_b32 s[[PTR_LO:[0-9]+]], v{{[0-9]+}}
 ; CI: v_readfirstlane_b32 s[[PTR_HI:[0-9]+]], v{{[0-9]+}}
-; CI-DAG: buffer_store_dword
 ; CI-DAG: s_load_dword s{{[0-9]+}}, s{{\[}}[[PTR_LO]]:[[PTR_HI]]{{\]}}, 0x1
 ; CI-DAG: s_load_dword s{{[0-9]+}}, s{{\[}}[[PTR_LO]]:[[PTR_HI]]{{\]}}, 0x2
 ; CI: buffer_store_dword
 define void @reorder_constant_load_global_store_constant_load(i32 addrspace(1)* %out, i32 addrspace(1)* %gptr) #0 {
   %ptr0 = load i32 addrspace(2)*, i32 addrspace(2)* addrspace(3)* @stored_constant_ptr, align 8
 
   %ptr1 = getelementptr inbounds i32, i32 addrspace(2)* %ptr0, i64 1
   %ptr2 = getelementptr inbounds i32, i32 addrspace(2)* %ptr0, i64 2
define void @reorder_local_offsets(i32 addrspace(1)* nocapture %out, i32 addrspace(1)* noalias nocapture readnone %gptr, i32 addrspace(3)* noalias nocapture %ptr0) #0 {
 
   %add.0 = add nsw i32 %tmp2, %tmp1
   %add.1 = add nsw i32 %add.0, %tmp3
   store i32 %add.1, i32 addrspace(1)* %out, align 4
   ret void
 }
 
 ; FUNC-LABEL: @reorder_global_offsets
-; CI: buffer_store_dword {{v[0-9]+}}, {{s\[[0-9]+:[0-9]+\]}}, 0 offset:12
 ; CI: buffer_load_dword {{v[0-9]+}}, {{s\[[0-9]+:[0-9]+\]}}, 0 offset:400
 ; CI: buffer_load_dword {{v[0-9]+}}, {{s\[[0-9]+:[0-9]+\]}}, 0 offset:404
-; CI: buffer_store_dword {{v[0-9]+}}, {{s\[[0-9]+:[0-9]+\]}}, 0 offset:400
+; CI: buffer_store_dword {{v[0-9]+}}, {{s\[[0-9]+:[0-9]+\]}}, 0 offset:12
 ; CI: buffer_load_dword {{v[0-9]+}}, {{s\[[0-9]+:[0-9]+\]}}, 0 offset:12
+; CI: buffer_store_dword {{v[0-9]+}}, {{s\[[0-9]+:[0-9]+\]}}, 0 offset:400
 ; CI: buffer_store_dword {{v[0-9]+}}, {{s\[[0-9]+:[0-9]+\]}}, 0 offset:404
 ; CI: buffer_store_dword
 ; CI: s_endpgm
 define void @reorder_global_offsets(i32 addrspace(1)* nocapture %out, i32 addrspace(1)* noalias nocapture readnone %gptr, i32 addrspace(1)* noalias nocapture %ptr0) #0 {
   %ptr1 = getelementptr inbounds i32, i32 addrspace(1)* %ptr0, i32 3
   %ptr2 = getelementptr inbounds i32, i32 addrspace(1)* %ptr0, i32 100
   %ptr3 = getelementptr inbounds i32, i32 addrspace(1)* %ptr0, i32 101
 

llvm/trunk/utils/TableGen/CodeGenDAGPatterns.cpp

void AnalyzeNode(const TreePatternNode *N) {
     // Notice properties of the node.
     if (N->NodeHasProperty(SDNPMayStore, CDP)) mayStore = true;
     if (N->NodeHasProperty(SDNPMayLoad, CDP)) mayLoad = true;
     if (N->NodeHasProperty(SDNPSideEffect, CDP)) hasSideEffects = true;
     if (N->NodeHasProperty(SDNPVariadic, CDP)) isVariadic = true;
 
     if (const CodeGenIntrinsic *IntInfo = N->getIntrinsicInfo(CDP)) {
       // If this is an intrinsic, analyze it.
-      if (IntInfo->ModRef >= CodeGenIntrinsic::ReadArgMem)
+      if (IntInfo->ModRef & CodeGenIntrinsic::MR_Ref)
         mayLoad = true;// These may load memory.
 
-      if (IntInfo->ModRef >= CodeGenIntrinsic::ReadWriteArgMem)
+      if (IntInfo->ModRef & CodeGenIntrinsic::MR_Mod)
         mayStore = true;// Intrinsics that can write to memory are 'mayStore'.
 
       if (IntInfo->ModRef >= CodeGenIntrinsic::ReadWriteMem)
-        // WriteMem intrinsics can have other strange effects.
+        // ReadWriteMem intrinsics can have other strange effects.
         hasSideEffects = true;
     }
   }
 
 };
 
 static bool InferFromPattern(CodeGenInstruction &InstInfo,
                              const InstAnalyzer &PatInfo,

llvm/trunk/utils/TableGen/CodeGenIntrinsics.h

struct IntrinsicSignature {
       std::vector<MVT::SimpleValueType> ParamVTs;
 
       /// ParamTypeDefs - The records for each parameter type.
       std::vector<Record*> ParamTypeDefs;
     };
 
     IntrinsicSignature IS;
 
-    // Memory mod/ref behavior of this intrinsic.
-    enum ModRefKind {
-      NoMem, ReadArgMem, ReadMem, ReadWriteArgMem, ReadWriteMem
+    /// Bit flags describing the type (ref/mod) and location of memory
+    /// accesses that may be performed by the intrinsics. Analogous to
+    /// \c FunctionModRefBehaviour.
+    enum ModRefBits {
+      /// The intrinsic may access memory anywhere, i.e. it is not restricted
+      /// to access through pointer arguments.
+      MR_Anywhere = 1,
+
+      /// The intrinsic may read memory.
+      MR_Ref = 2,
+
+      /// The intrinsic may write memory.
+      MR_Mod = 4,
+
+      /// The intrinsic may both read and write memory.
+      MR_ModRef = MR_Ref | MR_Mod,
+    };
+
+    /// Memory mod/ref behavior of this intrinsic, corresponding to
+    /// intrinsic properties (IntrReadMem, IntrReadArgMem, etc.).
+    enum ModRefBehavior {
+      NoMem = 0,
+      ReadArgMem = MR_Ref,
+      ReadMem = MR_Ref | MR_Anywhere,
+      WriteArgMem = MR_Mod,
+      WriteMem = MR_Mod | MR_Anywhere,
+      ReadWriteArgMem = MR_ModRef,
+      ReadWriteMem = MR_ModRef | MR_Anywhere,
     };
-    ModRefKind ModRef;
+    ModRefBehavior ModRef;
 
     /// This is set to true if the intrinsic is overloaded by its argument
     /// types.
     bool isOverloaded;
 
     /// isCommutative - True if the intrinsic is commutative.
     bool isCommutative;
 

llvm/trunk/utils/TableGen/CodeGenTarget.cpp

assert(Property->isSubClassOf("IntrinsicProperty") &&
            "Expected a property!");
 
     if (Property->getName() == "IntrNoMem")
       ModRef = NoMem;
     else if (Property->getName() == "IntrReadArgMem")
       ModRef = ReadArgMem;
     else if (Property->getName() == "IntrReadMem")
       ModRef = ReadMem;
+    else if (Property->getName() == "IntrWriteMem")
+      ModRef = WriteMem;
+    else if (Property->getName() == "IntrWriteArgMem")
+      ModRef = WriteArgMem;
     else if (Property->getName() == "IntrReadWriteArgMem")
       ModRef = ReadWriteArgMem;
     else if (Property->getName() == "Commutative")
       isCommutative = true;
     else if (Property->getName() == "Throws")
       canThrow = true;
     else if (Property->getName() == "IntrNoDuplicate")
       isNoDuplicate = true;

llvm/trunk/utils/TableGen/IntrinsicEmitter.cpp

bool operator()(const CodeGenIntrinsic *L, const CodeGenIntrinsic *R) const {
 
     if (L->isNoReturn != R->isNoReturn)
       return R->isNoReturn;
 
     if (L->isConvergent != R->isConvergent)
       return R->isConvergent;
 
     // Try to order by readonly/readnone attribute.
-    CodeGenIntrinsic::ModRefKind LK = L->ModRef;
-    CodeGenIntrinsic::ModRefKind RK = R->ModRef;
+    CodeGenIntrinsic::ModRefBehavior LK = L->ModRef;
+    CodeGenIntrinsic::ModRefBehavior RK = R->ModRef;
     if (LK != RK) return (LK > RK);
 
     // Order by argument attributes.
     // This is reliable because each side is already sorted internally.
     return (L->ArgumentAttributes < R->ArgumentAttributes);
   }
 };
 } // End anonymous namespace
if (!intrinsic.canThrow ||
         OS << "Attribute::ReadOnly,";
         OS << "Attribute::ArgMemOnly";
         break;
       case CodeGenIntrinsic::ReadMem:
         if (addComma)
           OS << ",";
         OS << "Attribute::ReadOnly";
         break;
+      case CodeGenIntrinsic::WriteArgMem:
       case CodeGenIntrinsic::ReadWriteArgMem:
         if (addComma)
           OS << ",";
         OS << "Attribute::ArgMemOnly";
         break;
+      case CodeGenIntrinsic::WriteMem:
       case CodeGenIntrinsic::ReadWriteMem:
         break;
       }
       OS << "};\n";
       OS << "      AS[" << numAttrs++ << "] = AttributeSet::get(C, "
          << "AttributeSet::FunctionIndex, Atts);\n";
     }