intrinsics attribute opt-out list proposal.

[sstefan1]

Summary

This patch introduces an opt-out attribute list for intrinsics, discussed in D65377.
This should make it easier to annotate intrinsics with attributes that apply to most of them.

[arsenm]

llvm/utils/TableGen/CodeGenTarget.cpp

770–771

I think you could avoid touching tablegen by implementing a filter in the tablegen source

[sstefan1]

llvm/utils/TableGen/CodeGenTarget.cpp

770–771

I'm not really sure how to do that in tablegen source. Not really used beyond adding properties to intrinsics.

Is there a reason for avoiding current approach?

[arsenm]

llvm/utils/TableGen/CodeGenTarget.cpp

770–771

You could do something like:

class Intrinsic {
  IntrProperties
  OutOutIntrProperties
   IntrPropertiesImpl = !foldl(something overly complicated to filter out OutIntrProperties)

}

I'm pretty confident we have all the necessary mechanics in tablegen to implement this, but it would be ugly. We probably don't have a nice way to test if an element is in a list, but it's possible.

I think just generally avoiding hardcoded things in tablegen is a good idea.

[sstefan1]

• revisit this patch with different implementation

[sstefan1]

Sorry this took so long.

I've implemented some sort of filter in tablegen source.

I've modified llvm.memset as an example how the opt-out list would look like. If this approach seems reasonable, I'll look what other tests have changed and need to be updated.

[arsenm]

llvm/include/llvm/IR/Intrinsics.td

122

This should be spilt to a separate change

354

I think this strategy doesn't actually work, given the prevalence of the error prone global let blocks. I've fixed many bugs from let Soemthing in :{} sections overwriting the inner members. I think the only way short of redesigning the language to have some kind of additive let to add to lists, I think the safest way to do this is to have tablegen handle the opt-out list

[sstefan1]

llvm/include/llvm/IR/Intrinsics.td

354

I'm little bit confused. Are you suggesting going back to the original approach?

[arsenm]

llvm/include/llvm/IR/Intrinsics.td

354

Yes, but I can't really decide which is better. Overriding class fields is generally a problem in tablegen though

[sstefan1]

Introduction of IsDefault field in IntrinsicProperty as suggested by @nhaehnle.

[sstefan1]

For now I've only set IntrNoSync to be default and changed memset to opt-out of IntrNoSync just as an example. Maybe the part where attributes are actually made default should be done in another patch?

Is there anyone else that should be added as a reviewer?

[jdoerfert]

One way to avoid some of the complexity would be to opt-out of everything with a single bit. So if this is set, you don't get any default attributes. Otherwise you get all.

When we settled on a scheme we need to send another email telling people we'll activate it for NoSycn "soon" and they should check their target intrinsics appropriately. For the non-target ones, opt-out of NoSync for the following intrinsics (I mention part of their name), roughly in order: gc (garbage collection), objc (because I don't know any better), read/write register, instprof, _eh_ (because I don't know any better), again _gc_, _coro_ (maybe), deoptimize, _clear_cache (as it probably defeats the purpose), hwasan, xray, *atomic

[sstefan1]

In D70365#2121590, @jdoerfert wrote:

One way to avoid some of the complexity would be to opt-out of everything with a single bit. So if this is set, you don't get any default attributes. Otherwise you get all.

Agreed, that could be another way to go about this. By now, I'm not really sure what is the best out of all the options. What do others think?

When we settled on a scheme we need to send another email telling people we'll activate it for NoSycn "soon" and they should check their target intrinsics appropriately.

Yes, that's why I suggested leaving that for another patch.

[efriedma]

One way to avoid some of the complexity would be to opt-out of everything with a single bit. So if this is set, you don't get any default attributes. Otherwise you get all.

Probably depends on what the initial list looks like. I guess something like "nosync, norecurse, nounwind, willreturn" is reasonable for most intrinsics. Given that list, probably an intrinsic that wants to opt-out of one of those will want to opt-out of all of them.

On a related note, we might want to consider doing something for pointer/memory handling as well, since that's currently pretty awkward. Most pointer arguments should be nocapture/nofree, but we don't really mark that consistently.

[sstefan1]

In D70365#2123539, @efriedma wrote:

On a related note, we might want to consider doing something for pointer/memory handling as well, since that's currently pretty awkward. Most pointer arguments should be nocapture/nofree, but we don't really mark that consistently.

I guess something similar can be done with pointer arguments as well. I can take a look at that once we settle on the approach here.

[jdoerfert]

In D70365#2123539, @efriedma wrote:

One way to avoid some of the complexity would be to opt-out of everything with a single bit. So if this is set, you don't get any default attributes. Otherwise you get all.

Probably depends on what the initial list looks like. I guess something like "nosync, norecurse, nounwind, willreturn" is reasonable for most intrinsics. Given that list, probably an intrinsic that wants to opt-out of one of those will want to opt-out of all of them.

That was what I would expect too. The above list might be extended with nofree but conceptually we might want to start with a single boolean to opt-out of everything. As we move on, we might go for a list or "categories", let's see.

@sstefan1 could u update the patch? We then send another email out with the list of 5 attributes we will make opt-out for intrinsics. A week or so later, assuming no one objected too much, we'll land it. In the meantime we need to update the target independent ones to opt-out appropriately. I guess my nosync list will catch almost all of them already but we'll have to give it another go as the patch is updated. WDYT?

[sstefan1]

DisableDefaultAttributes flag instead of opt-out list

[sstefan1]

In D70365#2128556, @jdoerfert wrote:

In D70365#2123539, @efriedma wrote:

@sstefan1 could u update the patch? We then send another email out with the list of 5 attributes we will make opt-out for intrinsics. A week or so later, assuming no one objected too much, we'll land it. In the meantime we need to update the target independent ones to opt-out appropriately. I guess my nosync list will catch almost all of them already but we'll have to give it another go as the patch is updated. WDYT?

I was under the impression that the attributes will be made default in another patch. Right now we don't opt out of the default attributes, but none are set to be default yet.
If it is better to do this in this patch, no problem. I can send an email, on Monday maybe?

[sstefan1]

small fix

[jdoerfert]

looks good, we should get the follow up ready that enables 1-5 default attributes

[sstefan1]

Ping.
Can we move forward with this approach?

[arsenm]

llvm/include/llvm/IR/Intrinsics.td

344

Should add a comment documenting this

[sstefan1]

add comment for DisableDefaultAttributes flag.

[sstefan1]

update affected tests.

[mehdi_amini]

After this patch, llvm-tblgen in debug mode is 10x slower:
Before:

$ time bin/llvm-tblgen -gen-global-isel -I ../llvm/test/TableGen/../../include -I ..//llvm/test/TableGen/Common -optimize-match-table=true ../llvm/test/TableGen/GlobalISelEmitter.td -o /tmp/test.cpp

real	0m1.238s
user	0m0.970s
sys	0m0.274s

After:

$ time bin/llvm-tblgen -gen-global-isel -I ../llvm/test/TableGen/../../include -I ..//llvm/test/TableGen/Common -optimize-match-table=true ../llvm/test/TableGen/GlobalISelEmitter.td -o /tmp/test.cpp

real	0m11.671s
user	0m11.607s
sys	0m0.067s

[jdoerfert]

In D70365#2227627, @mehdi_amini wrote:

After this patch, llvm-tblgen in debug mode is 10x slower:

Let's fix this properly. Reverted in 2f38c755ba46d1357d79c922966d964694eb854e

[sstefan1]

fix slowdown issue

[sstefan1]

In D70365#2227627, @mehdi_amini wrote:

After this patch, llvm-tblgen in debug mode is 10x slower:
Before:

$ time bin/llvm-tblgen -gen-global-isel -I ../llvm/test/TableGen/../../include -I ..//llvm/test/TableGen/Common -optimize-match-table=true ../llvm/test/TableGen/GlobalISelEmitter.td -o /tmp/test.cpp

real	0m1.238s
user	0m0.970s
sys	0m0.274s

After:

$ time bin/llvm-tblgen -gen-global-isel -I ../llvm/test/TableGen/../../include -I ..//llvm/test/TableGen/Common -optimize-match-table=true ../llvm/test/TableGen/GlobalISelEmitter.td -o /tmp/test.cpp

real	0m11.671s
user	0m11.607s
sys	0m0.067s

Thanks for reporting this.
Numbers are similar in the release mode as well. The problem was that we go through all IntrProperties for every Intrinsic in search for ones that are default. However that can be done only once.

With reverted patch time in release was 9.43s for me, now it is 0.16s.

I'd need another approval from @arsenm or @nhaehnle.

[sstefan1]

Ping.

Does the fix for the performance issue seem ok?

[jdoerfert]

LGTM.

llvm/include/llvm/IR/Intrinsics.td

 
 include "llvm/CodeGen/ValueTypes.td"
 include "llvm/CodeGen/SDNodeProperties.td"
 
 //===----------------------------------------------------------------------===//
 //  Properties we keep track of for intrinsics.
 //===----------------------------------------------------------------------===//
 
-class IntrinsicProperty;
+class IntrinsicProperty<bit is_default = 0> {
+  bit IsDefault = is_default;
+}
 
 // Intr*Mem - Memory properties.  If no property is set, the worst case
 // is assumed (it may read and write any memory it can get access to and it may
 // have other side effects).
 
 // IntrNoMem - The intrinsic does not access memory or have any other side
 // effects.  It may be CSE'd deleted if dead, etc.
 def IntrNoMem : IntrinsicProperty;

 // ReadNone - The specified argument pointer is not dereferenced by the
 // intrinsic.
 class ReadNone<AttrIndex idx> : IntrinsicProperty {
   int ArgNo = idx.Value;
 }
 
 def IntrNoReturn : IntrinsicProperty;
 
 def IntrNoFree : IntrinsicProperty;
 
 def IntrWillReturn : IntrinsicProperty;
 
 // IntrCold - Calls to this intrinsic are cold.
 // Parallels the cold attribute on LLVM IR functions.
 def IntrCold : IntrinsicProperty;
 
 // IntrNoduplicate - Calls to this intrinsic cannot be duplicated.

 //  * ParamTypes is a list containing the parameter types expected for the
 //    intrinsic.
 //  * Properties can be set to describe the behavior of the intrinsic.
 //
 class Intrinsic<list<LLVMType> ret_types,
                 list<LLVMType> param_types = [],
                 list<IntrinsicProperty> intr_properties = [],
                 string name = "",
-                list<SDNodeProperty> sd_properties = []> : SDPatternOperator {
+                list<SDNodeProperty> sd_properties = [],
+                bit disable_default_attributes = 0> : SDPatternOperator {
   string LLVMName = name;
   string TargetPrefix = "";   // Set to a prefix for target-specific intrinsics.
   list<LLVMType> RetTypes = ret_types;
   list<LLVMType> ParamTypes = param_types;
   list<IntrinsicProperty> IntrProperties = intr_properties;
   let Properties = sd_properties;
 
+  // Disable applying IntrinsicProperties that are marked default with
+  // IntrinsicProperty<1>
+  bit DisableDefaultAttributes = disable_default_attributes;
+
   bit isTarget = 0;
 }
 
 /// GCCBuiltin - If this intrinsic exactly corresponds to a GCC builtin, this
 /// specifies the name of the builtin.  This provides automatic CBE and CFE
 /// support.
 class GCCBuiltin<string name> {
   string GCCBuiltinName = name;
 }
 
 class MSBuiltin<string name> {
   string MSBuiltinName = name;
 }
 

llvm/utils/TableGen/CodeGenIntrinsics.h

struct CodeGenIntrinsic {
   };
 
   std::vector<ArgAttribute> ArgumentAttributes;
 
   bool hasProperty(enum SDNP Prop) const {
     return Properties & (1 << Prop);
   }
 
+  /// Goes through all IntrProperties and sets to true ones that have IsDefault
+  /// value set to true.
+  void setDefaultProperties(Record *R);
+
+  /// Helper function to set property \p Name to true;
+  void setProperty(Record *R);
+
   /// Returns true if the parameter at \p ParamIdx is a pointer type. Returns
   /// false if the parameter is not a pointer, or \p ParamIdx is greater than
   /// the size of \p IS.ParamVTs.
   ///
   /// Note that this requires that \p IS.ParamVTs is available.
   bool isParamAPointer(unsigned ParamIdx) const;
 
   bool isParamImmArg(unsigned ParamIdx) const;

llvm/utils/TableGen/CodeGenTarget.cpp

for (unsigned i = 0, e = TypeList->size(); i != e; ++i) {
     if (VT == MVT::isVoid && i != e-1 /*void at end means varargs*/)
       PrintFatalError(DefLoc, "Intrinsic '" + DefName +
                                   " has void in result type list!");
 
     IS.ParamVTs.push_back(VT);
     IS.ParamTypeDefs.push_back(TyEl);
   }
 
+  // Set default properties to true.
+  setDefaultProperties(R);
+
   // Parse the intrinsic properties.
   ListInit *PropList = R->getValueAsListInit("IntrProperties");
   for (unsigned i = 0, e = PropList->size(); i != e; ++i) {
     Record *Property = PropList->getElementAsRecord(i);
     assert(Property->isSubClassOf("IntrinsicProperty") &&
            "Expected a property!");
 
-    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) | 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;
-    else if (Property->getName() == "IntrNoReturn")
-      isNoReturn = true;
-    else if (Property->getName() == "IntrNoSync")
-      isNoSync = true;
-    else if (Property->getName() == "IntrNoFree")
-      isNoFree = true;
-    else if (Property->getName() == "IntrWillReturn")
-      isWillReturn = true;
-    else if (Property->getName() == "IntrCold")
-      isCold = true;
-    else if (Property->getName() == "IntrSpeculatable")
-      isSpeculatable = true;
-    else if (Property->getName() == "IntrHasSideEffects")
-      hasSideEffects = true;
-    else if (Property->isSubClassOf("NoCapture")) {
-      unsigned ArgNo = Property->getValueAsInt("ArgNo");
-      ArgumentAttributes.emplace_back(ArgNo, NoCapture, 0);
-    } else if (Property->isSubClassOf("NoAlias")) {
-      unsigned ArgNo = Property->getValueAsInt("ArgNo");
-      ArgumentAttributes.emplace_back(ArgNo, NoAlias, 0);
-    } else if (Property->isSubClassOf("Returned")) {
-      unsigned ArgNo = Property->getValueAsInt("ArgNo");
-      ArgumentAttributes.emplace_back(ArgNo, Returned, 0);
-    } else if (Property->isSubClassOf("ReadOnly")) {
-      unsigned ArgNo = Property->getValueAsInt("ArgNo");
-      ArgumentAttributes.emplace_back(ArgNo, ReadOnly, 0);
-    } else if (Property->isSubClassOf("WriteOnly")) {
-      unsigned ArgNo = Property->getValueAsInt("ArgNo");
-      ArgumentAttributes.emplace_back(ArgNo, WriteOnly, 0);
-    } else if (Property->isSubClassOf("ReadNone")) {
-      unsigned ArgNo = Property->getValueAsInt("ArgNo");
-      ArgumentAttributes.emplace_back(ArgNo, ReadNone, 0);
-    } else if (Property->isSubClassOf("ImmArg")) {
-      unsigned ArgNo = Property->getValueAsInt("ArgNo");
-      ArgumentAttributes.emplace_back(ArgNo, ImmArg, 0);
-    } else if (Property->isSubClassOf("Align")) {
-      unsigned ArgNo = Property->getValueAsInt("ArgNo");
-      uint64_t Align = Property->getValueAsInt("Align");
-      ArgumentAttributes.emplace_back(ArgNo, Alignment, Align);
-    } else
-      llvm_unreachable("Unknown property!");
-  }
+    setProperty(Property);
+  }
+
+  // Also record the SDPatternOperator Properties.
+  Properties = parseSDPatternOperatorProperties(R);
+
+  // Sort the argument attributes for later benefit.
+  llvm::sort(ArgumentAttributes);
+}
+
+void CodeGenIntrinsic::setDefaultProperties(Record *R) {
+  // opt-out of using default attributes.
+  if (R->getValueAsBit("DisableDefaultAttributes"))
+    return;
+
+  std::vector<Record *> Defs =
+      R->getRecords().getAllDerivedDefinitions("IntrinsicProperty");
+
+  for (Record *Rec : Defs)
+    if (Rec->getValueAsBit("IsDefault"))
+      setProperty(Rec);
+}
+
+void CodeGenIntrinsic::setProperty(Record *R) {
+  if (R->getName() == "IntrNoMem")
+  else if (R->getName() == "IntrReadMem")
+  else if (R->getName() == "IntrWriteMem")
+  else if (R->getName() == "IntrArgMemOnly")
+  else if (R->getName() == "IntrInaccessibleMemOnly")
+  else if (R->getName() == "IntrInaccessibleMemOrArgMemOnly")
+  else if (R->getName() == "Commutative")
+  else if (R->getName() == "Throws")
+  else if (R->getName() == "IntrNoDuplicate")
+  else if (R->getName() == "IntrConvergent")
+  else if (R->getName() == "IntrNoReturn")
+  else if (R->getName() == "IntrNoSync")
+  else if (R->getName() == "IntrNoFree")
+  else if (R->getName() == "IntrWillReturn")
+  else if (R->getName() == "IntrCold")
+  else if (R->getName() == "IntrSpeculatable")
+  else if (R->getName() == "IntrHasSideEffects")
+  else if (R->isSubClassOf("NoCapture")) {
+    unsigned ArgNo = R->getValueAsInt("ArgNo");
+  } else if (R->isSubClassOf("NoAlias")) {
+    unsigned ArgNo = R->getValueAsInt("ArgNo");
+  } else if (R->isSubClassOf("Returned")) {
+    unsigned ArgNo = R->getValueAsInt("ArgNo");
+  } else if (R->isSubClassOf("ReadOnly")) {
+    unsigned ArgNo = R->getValueAsInt("ArgNo");
+  } else if (R->isSubClassOf("WriteOnly")) {
+    unsigned ArgNo = R->getValueAsInt("ArgNo");
+  } else if (R->isSubClassOf("ReadNone")) {
+    unsigned ArgNo = R->getValueAsInt("ArgNo");
+  } else if (R->isSubClassOf("ImmArg")) {
+    unsigned ArgNo = R->getValueAsInt("ArgNo");
+  } else if (R->isSubClassOf("Align")) {
+    unsigned ArgNo = R->getValueAsInt("ArgNo");
+    uint64_t Align = R->getValueAsInt("Align");
 
-  // Also record the SDPatternOperator Properties.
-  Properties = parseSDPatternOperatorProperties(R);
-
-  // Sort the argument attributes for later benefit.
-  llvm::sort(ArgumentAttributes);
-}
-
 bool CodeGenIntrinsic::isParamAPointer(unsigned ParamIdx) const {
   if (ParamIdx >= IS.ParamVTs.size())
     return false;
   MVT ParamType = MVT(IS.ParamVTs[ParamIdx]);
   return ParamType == MVT::iPTR || ParamType == MVT::iPTRAny;
 }
 
 bool CodeGenIntrinsic::isParamImmArg(unsigned ParamIdx) const {
   // Convert argument index to attribute index starting from `FirstArgIndex`.
   ArgAttribute Val{ParamIdx + 1, ImmArg, 0};
   return std::binary_search(ArgumentAttributes.begin(),
                             ArgumentAttributes.end(), Val);
 }