review comments: remove attribute string generation

- convert to using a struct to store the active attributes to let us use
a table of bools instead of a string list of attributes

Author: inbelic

llvm/lib/Target/DirectX/DXILConstants.h

@@ -30,11 +30,24 @@ enum class OpParamType : unsigned {
 #include "DXILOperation.inc"
 };
 
-enum class Attribute : unsigned {
-#define DXIL_ATTRIBUTE(Name) Name,
+struct Attributes {
+#define DXIL_ATTRIBUTE(Name) bool Name = false;
 #include "DXILOperation.inc"
 };
 
+inline Attributes operator|(Attributes a, Attributes b) {
+  Attributes c;
+#define DXIL_ATTRIBUTE(Name) c.Name = a.Name | b.Name;
+#include "DXILOperation.inc"
+ return c;
+}
+
+inline Attributes &operator|=(Attributes &a, Attributes &b) {
+  a = a | b;
+  return a;
+}
+
+
 enum class Property : unsigned {
 #define DXIL_PROPERTY(Name) Name,
 #include "DXILOperation.inc"

llvm/lib/Target/DirectX/DXILOpBuilder.cpp

@@ -52,11 +52,6 @@ struct OpStage {
   uint32_t ValidStages;
 };
 
-struct OpAttribute {
-  Version DXILVersion;
-  llvm::SmallVector<dxil::Attribute> ValidAttrs;
-};
-
 static const char *getOverloadTypeName(OverloadKind Kind) {
   switch (Kind) {
   case OverloadKind::HALF:
@@ -158,7 +153,6 @@ struct OpCodeProperty {
   unsigned OpCodeClassNameOffset;
   llvm::SmallVector<OpOverload> Overloads;
   llvm::SmallVector<OpStage> Stages;
-  llvm::SmallVector<OpAttribute> Attributes;
   llvm::SmallVector<dxil::Property> Properties;
   int OverloadParamIndex; // parameter index which control the overload.
                           // When < 0, should be only 1 overload type.
@@ -368,18 +362,40 @@ static std::optional<size_t> getPropIndex(ArrayRef<T> PropList,
   return std::nullopt;
 }
 
-static void setDXILAttribute(CallInst *CI, dxil::Attribute Attr) {
-  switch (Attr) {
-  case dxil::Attribute::ReadNone:
-    return CI->setDoesNotAccessMemory();
-  case dxil::Attribute::ReadOnly:
-    return CI->setOnlyReadsMemory();
-  case dxil::Attribute::NoReturn:
-    return CI->setDoesNotReturn();
-  case dxil::Attribute::NoDuplicate:
-    return CI->setCannotDuplicate();
+constexpr static uint64_t computeSwitchEnum(dxil::OpCode OpCode, uint16_t VersionMajor, uint16_t VersionMinor) {
+  uint64_t OpCodePack = (uint64_t)OpCode;
+  return (OpCodePack << 32) | (VersionMajor << 16) | VersionMinor;
+}
+
+static dxil::Attributes getDXILAttributes(dxil::OpCode OpCode, VersionTuple DXILVersion) {
+  SmallVector<Version> Versions = {
+#define DXIL_VERSION(MAJOR, MINOR) {MAJOR, MINOR},
+#include "DXILOperation.inc"
+  };
+
+  dxil::Attributes Attributes;
+  for (auto Version : Versions) {
+    if (DXILVersion < VersionTuple(Version.Major, Version.Minor)) continue;
+    switch (computeSwitchEnum(OpCode, Version.Major, Version.Minor)) {
+#define DXIL_OP_ATTRIBUTES(OpCode, VersionMajor, VersionMinor, ...) \
+    case computeSwitchEnum(OpCode, VersionMajor, VersionMinor): { \
+      auto Other = dxil::Attributes{__VA_ARGS__}; \
+      Attributes |= Other; \
+      break; \
+    };
+#include "DXILOperation.inc"
+    }
   }
-  llvm_unreachable("Invalid function attribute specified for DXIL operation");
+  return Attributes;
+}
+
+static void setDXILAttributes(CallInst *CI, dxil::OpCode OpCode, VersionTuple DXILVersion) {
+  dxil::Attributes Attributes = getDXILAttributes(OpCode, DXILVersion);
+  if (Attributes.ReadNone) CI->setDoesNotAccessMemory();
+  if (Attributes.ReadOnly) CI->setOnlyReadsMemory();
+  if (Attributes.NoReturn) CI->setDoesNotReturn();
+  if (Attributes.NoDuplicate) CI->setCannotDuplicate();
+  return;
 }
 
 namespace llvm {
@@ -480,11 +496,7 @@ Expected<CallInst *> DXILOpBuilder::tryCreateOp(dxil::OpCode OpCode,
   CallInst *CI = IRB.CreateCall(DXILFn, OpArgs, Name);
 
   // We then need to attach available function attributes
-  for (auto OpAttr : Prop->Attributes)
-    if (VersionTuple(OpAttr.DXILVersion.Major, OpAttr.DXILVersion.Minor) <=
-        DXILVersion)
-      for (auto Attr : OpAttr.ValidAttrs)
-        setDXILAttribute(CI, Attr);
+  setDXILAttributes(CI, OpCode, DXILVersion);
 
   return CI;
 }

llvm/utils/TableGen/DXILEmitter.cpp

@@ -319,43 +319,6 @@ static std::string getStageMaskString(ArrayRef<const Record *> Recs) {
   return MaskString;
 }
 
-/// Return a string representation of valid attribute information denoted
-// by input records
-//
-/// \param Recs A vector of records of TableGen Attribute records
-/// \return std::string string representation of attributes list string
-///         predicated by DXIL Version. E.g.,
-//          {{{1, 0}, {Attr1, ...}}, {{1, 2}, {Attr2, ...}}, ...}
-static std::string getAttributeListString(ArrayRef<const Record *> Recs) {
-  std::string ListString = "";
-  std::string Prefix = "";
-  ListString.append("{");
-
-  for (const auto *Rec : Recs) {
-    unsigned Major = Rec->getValueAsDef("dxil_version")->getValueAsInt("Major");
-    unsigned Minor = Rec->getValueAsDef("dxil_version")->getValueAsInt("Minor");
-    ListString.append(Prefix)
-        .append("{{")
-        .append(std::to_string(Major))
-        .append(", ")
-        .append(std::to_string(Minor).append("}, {"));
-
-    std::string CommaPrefix = "";
-    auto Attrs = Rec->getValueAsListOfDefs("fn_attrs");
-    for (const auto *Attr : Attrs) {
-      ListString.append(CommaPrefix)
-          .append("dxil::Attribute::")
-          .append(Attr->getName());
-      CommaPrefix = ", ";
-    }
-    ListString.append("}"); // End of Attrs
-    ListString.append("}"); // End of Rec
-    Prefix = ", ";
-  }
-  ListString.append("}"); // End of List
-  return ListString;
-}
-
 /// Return a string representation of valid property information denoted
 // by input records
 //
@@ -378,6 +341,20 @@ static std::string getPropertyListString(ArrayRef<const Record *> Recs) {
   return ListString;
 }
 
+/// Emit a list valid DXIL Version records
+static void emitDXILVersions(const RecordKeeper &Records, raw_ostream &OS) {
+  OS << "#ifdef DXIL_VERSION\n";
+  for (const Record *Version : Records.getAllDerivedDefinitions("Version")) {
+      unsigned Major = Version->getValueAsInt("Major");
+      unsigned Minor = Version->getValueAsInt("Minor");
+      OS << "DXIL_VERSION(";
+      OS << std::to_string(Major) << ", " << std::to_string(Minor);
+      OS << ")\n";
+  }
+  OS << "#undef DXIL_VERSION\n";
+  OS << "#endif\n\n";
+}
+
 /// Emit a mapping of DXIL opcode to opname
 static void emitDXILOpCodes(ArrayRef<DXILOperationDesc> Ops, raw_ostream &OS) {
   OS << "#ifdef DXIL_OPCODE\n";
@@ -416,6 +393,40 @@ static void emitDXILAttributes(const RecordKeeper &Records, raw_ostream &OS) {
   OS << "#endif\n\n";
 }
 
+// Determine which function attributes are set for a dxil version
+static bool attrIsDefined(std::vector<const Record *> Attrs, const Record *Attr) {
+  for (auto CurAttr : Attrs)
+    if (CurAttr->getName() == Attr->getName())
+      return true;
+  return false;
+}
+
+/// Emit a table of bools denoting a DXIL op's function attributes
+static void emitDXILOpAttributes(const RecordKeeper &Records,
+                                 ArrayRef<DXILOperationDesc> Ops,
+                                 raw_ostream &OS) {
+  OS << "#ifdef DXIL_OP_ATTRIBUTES\n";
+  for (const auto &Op : Ops) {
+    for (const auto *Rec : Op.AttrRecs) {
+      unsigned Major = Rec->getValueAsDef("dxil_version")->getValueAsInt("Major");
+      unsigned Minor = Rec->getValueAsDef("dxil_version")->getValueAsInt("Minor");
+      OS << "DXIL_OP_ATTRIBUTES(dxil::OpCode::" << Op.OpName << ", ";
+      OS << std::to_string(Major) << ", " << std::to_string(Minor);
+      auto Attrs = Rec->getValueAsListOfDefs("fn_attrs");
+      for (const Record *Attr : Records.getAllDerivedDefinitions("DXILAttribute")) {
+        std::string HasAttr = ", false";
+        if (attrIsDefined(Attrs, Attr))
+          HasAttr = ", true";
+        OS << HasAttr;
+      }
+      OS << ")\n";
+    }
+    
+  }
+  OS << "#undef DXIL_OP_ATTRIBUTES\n";
+  OS << "#endif\n\n";
+}
+
 /// Emit a list of DXIL op properties and their query functions
 static void emitDXILProperties(const RecordKeeper &Records, raw_ostream &OS) {
   // Generate their definitions
@@ -544,7 +555,6 @@ static void emitDXILOperationTable(ArrayRef<DXILOperationDesc> Ops,
        << OpClassStrings.get(Op.OpClass.data()) << ", "
        << getOverloadMaskString(Op.OverloadRecs) << ", "
        << getStageMaskString(Op.StageRecs) << ", "
-       << getAttributeListString(Op.AttrRecs) << ", "
        << getPropertyListString(Op.PropRecs) << ", " << Op.OverloadParamIndex
        << " }";
     Prefix = ",\n";
@@ -647,10 +657,12 @@ static void emitDxilOperation(const RecordKeeper &Records, raw_ostream &OS) {
     PrevOp = Desc.OpCode;
   }
 
+  emitDXILVersions(Records, OS);
   emitDXILOpCodes(DXILOps, OS);
   emitDXILOpClasses(Records, OS);
   emitDXILOpParamTypes(Records, OS);
   emitDXILAttributes(Records, OS);
+  emitDXILOpAttributes(Records, DXILOps, OS);
   emitDXILProperties(Records, OS);
   emitDXILOpFunctionTypes(DXILOps, OS);
   emitDXILIntrinsicArgSelectTypes(Records, OS);