Add IntrInaccessibleMemOnly property for intrinsics

[andrew.w.kaylor]

Summary

This property corresponds directly to the inaccessiblememonly function attribute.

I intend to use this attribute with the experimental constrained FP intrinsics that I will be adding soon, but it seemed best to have these changes reviewed separately.

I considered adding this attribute to the @llvm.assume() intrinsic as Hal suggested in the comments on D26382, but that would have resulted in changes to aliasing behavior which is specifically tested for in test/Analysis/BasicAA/assume.ll and I am not familiar enough with that intrinsic to be comfortable judging whether or not that is acceptable. Consequently, I have no direct test for this change set. I am, of course, open to guidance.

A notable piece of code that I did not change is the InstAnalyzer::AnalyzeNode() function in utils/TableGen/CodeGenDAGPatterns.cpp. That function contains the following code:

if (const CodeGenIntrinsic *IntInfo = N->getIntrinsicInfo(CDP)) {
  // If this is an intrinsic, analyze it.
  if (IntInfo->ModRef & CodeGenIntrinsic::MR_Ref)
    mayLoad = true;// These may load memory.

  if (IntInfo->ModRef & CodeGenIntrinsic::MR_Mod)
    mayStore = true;// Intrinsics that can write to memory are 'mayStore'.

  if (IntInfo->ModRef >= CodeGenIntrinsic::ReadWriteMem)
    // ReadWriteMem intrinsics can have other strange effects.
    hasSideEffects = true;
}

This code will not set the 'hasSideEffects' flag for intrinsics that use the IntrInaccessibleMemOnly property, but it currently does not set that flag for intrinsics that use the IntrArgMemOnly property. This seems wrong to me in both cases, but I decided to defer to the existing logic over my own judgment pending review.

[hfinkel]

I considered adding this attribute to the @llvm.assume() intrinsic as Hal suggested in the comments on D26382, but that would have resulted in changes to aliasing behavior which is specifically tested for in test/Analysis/BasicAA/assume.ll and I am not familiar enough with that intrinsic to be comfortable judging whether or not that is acceptable. Consequently, I have no direct test for this change set. I am, of course, open to guidance.

I don't see why they should be different. Please feel free to update the test case and I'll review it -- or explain why the semantics are different.

This code will not set the 'hasSideEffects' flag for intrinsics that use the IntrInaccessibleMemOnly property, but it currently does not set that flag for intrinsics that use the IntrArgMemOnly property. This seems wrong to me in both cases, but I decided to defer to the existing logic over my own judgment pending review.

hasSideEffects is somewhat misnamed. It really means "has otherwise-unmodeled side effects", so not setting it seems correct.

include/llvm/IR/Intrinsics.td

48

We should go ahead and add InaccessibleMemOrArgMemOnly too while we're here.

[andrew.w.kaylor]

I added the IntrInaccessibleMemOrArgMemOnly property, and updated the llvm.assume intrinsic to use IntrInaccessibleMemOnly.

It turns out that I was wrong about this changing the aliasing behavior, at least with respect to BasicAA, because BasicAA specifically looks for llvm.assume and reports that it does not alias with any memory location or function call. The reason I thought it would be a change is because I expected llvm.assume with the new property to alias in the same way any other inaccessiblememonly function would, meaning it would report "Both ModRef" for any function call except calls to argmemonly functions.

It's not clear to me which other existing intrinsics should be using the new properties.

[andrew.w.kaylor]

Ping.

[hfinkel]

In D26485#591966, @andrew.w.kaylor wrote:

I added the IntrInaccessibleMemOrArgMemOnly property, and updated the llvm.assume intrinsic to use IntrInaccessibleMemOnly.

It turns out that I was wrong about this changing the aliasing behavior, at least with respect to BasicAA, because BasicAA specifically looks for llvm.assume and reports that it does not alias with any memory location or function call. The reason I thought it would be a change is because I expected llvm.assume with the new property to alias in the same way any other inaccessiblememonly function would, meaning it would report "Both ModRef" for any function call except calls to argmemonly functions.

Ah, you're right. Nevermind, then. Adding the attribute to assume does not help because we still want assume not to alias with other function calls, and so it still needs special handling. Please remove the change to assume as it does not let us remove the special-case behavior in BasicAA.

It's not clear to me which other existing intrinsics should be using the new properties.

[andrew.w.kaylor]

Removed IntrInaccessibleMemOnly from llvm.assume.

[hfinkel]

LGTM

include/llvm/IR/Intrinsics.td

 // to this intrinsics may be removed.
 def IntrWriteMem : IntrinsicProperty;
 
 // IntrArgMemOnly - This intrinsic only accesses memory that its pointer-typed
 // argument(s) points to, but may access an unspecified amount. Other than
 // reads from and (possibly volatile) writes to memory, it has no side effects.
 def IntrArgMemOnly : IntrinsicProperty;
 
+// IntrInaccessibleMemOnly -- This intrinsic only accesses memory that is not
+// accessible by the module being compiled. This is a weaker form of IntrNoMem.
+def IntrInaccessibleMemOnly : IntrinsicProperty;
+
+// IntrInaccessibleMemOrArgMemOnly -- This intrinsic only accesses memory that
+// its pointer-typed arguments point to or memory that is not accessible
+// by the module being compiled. This is a weaker form of IntrArgMemOnly.
+def IntrInaccessibleMemOrArgMemOnly : IntrinsicProperty;
+
 // Commutative - This intrinsic is commutative: X op Y == Y op X.
 def Commutative : IntrinsicProperty;
 
 // Throws - This intrinsic can throw.
 def Throws : IntrinsicProperty;
 
 // NoCapture - The specified argument pointer is not captured by the intrinsic.
 class NoCapture<int argNo> : IntrinsicProperty {

utils/TableGen/CodeGenIntrinsics.h

struct CodeGenIntrinsic {
   };
 
   IntrinsicSignature IS;
 
   /// 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 that is otherwise inaccessible via
+    /// LLVM IR.
+    MR_InaccessibleMem = 1,
+
+    /// The intrinsic may access memory through pointer arguments.
+    /// LLVM IR.
+    MR_ArgMem = 2,
+
     /// The intrinsic may access memory anywhere, i.e. it is not restricted
     /// to access through pointer arguments.
-    MR_Anywhere = 1,
+    MR_Anywhere = 4 | MR_ArgMem | MR_InaccessibleMem,
 
     /// The intrinsic may read memory.
-    MR_Ref = 2,
+    MR_Ref = 8,
 
     /// The intrinsic may write memory.
-    MR_Mod = 4,
+    MR_Mod = 16,
 
     /// 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, IntrArgMemOnly, etc.).
   enum ModRefBehavior {
     NoMem = 0,
-    ReadArgMem = MR_Ref,
+    ReadArgMem = MR_Ref | MR_ArgMem,
+    ReadInaccessibleMem = MR_Ref | MR_InaccessibleMem,
+    ReadInaccessibleMemOrArgMem = MR_Ref | MR_ArgMem | MR_InaccessibleMem,
     ReadMem = MR_Ref | MR_Anywhere,
-    WriteArgMem = MR_Mod,
+    WriteArgMem = MR_Mod | MR_ArgMem,
+    WriteInaccessibleMem = MR_Mod | MR_InaccessibleMem,
+    WriteInaccessibleMemOrArgMem = MR_Mod | MR_ArgMem | MR_InaccessibleMem,
     WriteMem = MR_Mod | MR_Anywhere,
-    ReadWriteArgMem = MR_ModRef,
+    ReadWriteArgMem = MR_ModRef | MR_ArgMem,
+    ReadWriteInaccessibleMem = MR_ModRef | MR_InaccessibleMem,
+    ReadWriteInaccessibleMemOrArgMem = MR_ModRef | MR_ArgMem |
+                                       MR_InaccessibleMem,
     ReadWriteMem = MR_ModRef | MR_Anywhere,
   };
   ModRefBehavior ModRef;
 
   /// This is set to true if the intrinsic is overloaded by its argument
   /// types.
   bool isOverloaded;
 

utils/TableGen/CodeGenTarget.cpp

for (unsigned i = 0, e = PropList->size(); i != e; ++i) {
 
     if (Property->getName() == "IntrNoMem")
       ModRef = NoMem;
     else if (Property->getName() == "IntrReadMem")
       ModRef = ModRefBehavior(ModRef & ~MR_Mod);
     else if (Property->getName() == "IntrWriteMem")
       ModRef = ModRefBehavior(ModRef & ~MR_Ref);
     else if (Property->getName() == "IntrArgMemOnly")
-      ModRef = ModRefBehavior(ModRef & ~MR_Anywhere);
+      ModRef = ModRefBehavior((ModRef & ~MR_Anywhere) | MR_ArgMem);
+    else if (Property->getName() == "IntrInaccessibleMemOnly")
+      ModRef = ModRefBehavior((ModRef & ~MR_Anywhere) | MR_InaccessibleMem);
+    else if (Property->getName() == "IntrInaccessibleMemOrArgMemOnly")
+      ModRef = ModRefBehavior((ModRef & ~MR_Anywhere) | MR_ArgMem |
+                              MR_InaccessibleMem);
     else if (Property->getName() == "Commutative")
       isCommutative = true;
     else if (Property->getName() == "Throws")
       canThrow = true;
     else if (Property->getName() == "IntrNoDuplicate")
       isNoDuplicate = true;
     else if (Property->getName() == "IntrConvergent")
       isConvergent = true;

utils/TableGen/IntrinsicEmitter.cpp

if (!intrinsic.canThrow ||
         OS << "Attribute::ReadOnly,";
         OS << "Attribute::ArgMemOnly";
         break;
       case CodeGenIntrinsic::ReadMem:
         if (addComma)
           OS << ",";
         OS << "Attribute::ReadOnly";
         break;
+      case CodeGenIntrinsic::ReadInaccessibleMem:
+        if (addComma)
+          OS << ",";
+        OS << "Attribute::ReadOnly,";
+        OS << "Attribute::InaccessibleMemOnly";
+        break;
+      case CodeGenIntrinsic::ReadInaccessibleMemOrArgMem:
+        if (addComma)
+          OS << ",";
+        OS << "Attribute::ReadOnly,";
+        OS << "Attribute::InaccessibleMemOrArgMemOnly";
+        break;
       case CodeGenIntrinsic::WriteArgMem:
         if (addComma)
           OS << ",";
         OS << "Attribute::WriteOnly,";
         OS << "Attribute::ArgMemOnly";
         break;
       case CodeGenIntrinsic::WriteMem:
         if (addComma)
           OS << ",";
         OS << "Attribute::WriteOnly";
         break;
+      case CodeGenIntrinsic::WriteInaccessibleMem:
+        if (addComma)
+          OS << ",";
+        OS << "Attribute::WriteOnly,";
+        OS << "Attribute::InaccessibleMemOnly";
+        break;
+      case CodeGenIntrinsic::WriteInaccessibleMemOrArgMem:
+        if (addComma)
+          OS << ",";
+        OS << "Attribute::WriteOnly,";
+        OS << "Attribute::InaccessibleMemOrArgOnly";
+        break;
       case CodeGenIntrinsic::ReadWriteArgMem:
         if (addComma)
           OS << ",";
         OS << "Attribute::ArgMemOnly";
         break;
+      case CodeGenIntrinsic::ReadWriteInaccessibleMem:
+        if (addComma)
+          OS << ",";
+        OS << "Attribute::InaccessibleMemOnly";
+        break;
+      case CodeGenIntrinsic::ReadWriteInaccessibleMemOrArgMem:
+        if (addComma)
+          OS << ",";
+        OS << "Attribute::InaccessibleMemOrArgMemOnly";
       case CodeGenIntrinsic::ReadWriteMem:
         break;
       }
       OS << "};\n";
       OS << "      AS[" << numAttrs++ << "] = AttributeSet::get(C, "
          << "AttributeSet::FunctionIndex, Atts);\n";
     }