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

65

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

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

[nhaehnle]

Changes to the ModRefBehavior enum in TableGen.

[nhaehnle]

utils/TableGen/CodeGenIntrinsics.h

65

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

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

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

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.

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 writes may
+// be volatile, but 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;

include/llvm/IR/IntrinsicsAMDGPU.td

 def int_amdgcn_buffer_store_format : Intrinsic <
   [],
   [llvm_anyfloat_ty,  // vdata(VGPR) -- can currently only select 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]>;
 
 class AMDGPUBufferAtomic : Intrinsic <
   [llvm_i32_ty],
   [llvm_i32_ty,       // vdata(VGPR)
    llvm_v4i32_ty,     // rsrc(SGPR)
    llvm_i32_ty,       // vindex(VGPR)
    llvm_i32_ty,       // offset(SGPR/VGPR/imm)
    llvm_i1_ty],       // slc(imm)

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

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,

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;
 

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;

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 ||
       if (intrinsic.isConvergent) {
         if (addComma)
           OS << ",";
         OS << "Attribute::Convergent";
         addComma = true;
       }
 
       switch (intrinsic.ModRef) {
       case CodeGenIntrinsic::NoMem:
         if (addComma)
           OS << ",";
         OS << "Attribute::ReadNone";
         break;
       case CodeGenIntrinsic::ReadArgMem:
         if (addComma)
           OS << ",";
         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";
     }