[NFC] review comments:

- update nit from review comments
- fix some typos in comments
- add additional comments for clarity

Author: inbelic

llvm/lib/Target/DirectX/DXIL.td

@@ -270,18 +270,18 @@ def removed : DXILShaderStage;
 // DXIL Op attributes
 
 // A function attribute denotes that there is a corresponding LLVM function
-// attribute that will be set when building the DXIL op. The mapping for
-// non-trivial cases is defined by setDXILAttribute in DXILOpBuilder.cpp
+// attribute that will be set when building the DXIL op. The mapping is defined
+// by setDXILAttributes in DXILOpBuilder.cpp
 class DXILAttribute;
 
 def ReadNone : DXILAttribute;
 def ReadOnly : DXILAttribute;
 def NoDuplicate : DXILAttribute;
 def NoReturn : DXILAttribute;
 
-// A property is simply used to mark a DXIL op belongs to a sub-group of
-// DXIL ops, and it is used to query if a particular holds this property.
-// This is used for static analysis of DXIL ops.
+// A property is simply used to mark that a DXIL op belongs to a sub-group of
+// DXIL ops, and it is used to query if a particular op holds this property.
+// This is used for the static analysis of DXIL ops.
 class DXILProperty;
 
 def IsBarrier : DXILProperty;
@@ -797,6 +797,9 @@ def CreateHandle : DXILOp<57, createHandle> {
   let arguments = [Int8Ty, Int32Ty, Int32Ty, Int1Ty];
   let result = HandleTy;
   let stages = [Stages<DXIL1_0, [all_stages]>, Stages<DXIL1_6, [removed]>];
+  // NOTE: The ReadOnly attribute was set for consistency with DXC. However, it
+  // seems like ReadNone may more appropiately describe it. So noted to
+  // consider a change in the future
   let attributes = [Attributes<DXIL1_0, [ReadOnly]>];
 }
 

llvm/lib/Target/DirectX/DXILOpBuilder.cpp

@@ -361,15 +361,20 @@ static std::optional<size_t> getPropIndex(ArrayRef<T> PropList,
   return std::nullopt;
 }
 
+// Helper function to pack an OpCode and VersionTuple into a uint64_t for use
+// in a switch statement
 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;
 }
 
+// Retreive all the set attributes for a DXIL OpCode given the targeted
+// DXILVersion
 static dxil::Attributes getDXILAttributes(dxil::OpCode OpCode,
                                           VersionTuple DXILVersion) {
+  // Instantiate all versions to iterate through
   SmallVector<Version> Versions = {
 #define DXIL_VERSION(MAJOR, MINOR) {MAJOR, MINOR},
 #include "DXILOperation.inc"
@@ -379,6 +384,9 @@ static dxil::Attributes getDXILAttributes(dxil::OpCode OpCode,
   for (auto Version : Versions) {
     if (DXILVersion < VersionTuple(Version.Major, Version.Minor))
       continue;
+
+    // Switch through and match an OpCode with the specific version and set the
+    // corresponding flag(s) if available
     switch (computeSwitchEnum(OpCode, Version.Major, Version.Minor)) {
 #define DXIL_OP_ATTRIBUTES(OpCode, VersionMajor, VersionMinor, ...)            \
   case computeSwitchEnum(OpCode, VersionMajor, VersionMinor): {                \
@@ -392,6 +400,8 @@ static dxil::Attributes getDXILAttributes(dxil::OpCode OpCode,
   return Attributes;
 }
 
+// Retreive the set of DXIL Attributes given the version and map them to an
+// llvm function attribute that is set onto the instruction
 static void setDXILAttributes(CallInst *CI, dxil::OpCode OpCode,
                               VersionTuple DXILVersion) {
   dxil::Attributes Attributes = getDXILAttributes(OpCode, DXILVersion);

llvm/utils/TableGen/DXILEmitter.cpp

@@ -181,9 +181,8 @@ DXILOperationDesc::DXILOperationDesc(const Record *R) {
   Recs = R->getValueAsListOfDefs("properties");
 
   // Get property records
-  for (const Record *CR : Recs) {
+  for (const Record *CR : Recs)
     PropRecs.push_back(CR);
-  }
 
   // Get the operation class
   OpClass = R->getValueAsDef("OpClass")->getName();
@@ -371,7 +370,8 @@ static void emitDXILAttributes(const RecordKeeper &Records, raw_ostream &OS) {
   OS << "#endif\n\n";
 }
 
-// Determine which function attributes are set for a dxil version
+// Helper function to determine if the given Attr is defined in the vector
+// Attrs, by comparing the names
 static bool attrIsDefined(std::vector<const Record *> Attrs,
                           const Record *Attr) {
   for (auto CurAttr : Attrs)
@@ -384,6 +384,13 @@ static bool attrIsDefined(std::vector<const Record *> Attrs,
 static void emitDXILOpAttributes(const RecordKeeper &Records,
                                  ArrayRef<DXILOperationDesc> Ops,
                                  raw_ostream &OS) {
+  // A DXIL op can have multiple function attributes that are specific to a
+  // specific DXIL version and higher. AttrRecs models this by grouping the
+  // attributes by the versions. So we will output a macro for each version
+  // number with a table of bools in the following format:
+  //
+  //     OpName, VersionMajor, VersionMinor, FnAttr1, FnAttr2, ...
+  // Eg)    Abs,            1,            0,    true,   false, ...
   OS << "#ifdef DXIL_OP_ATTRIBUTES\n";
   for (const auto &Op : Ops) {
     for (const auto *Rec : Op.AttrRecs) {
@@ -393,7 +400,11 @@ static void emitDXILOpAttributes(const RecordKeeper &Records,
           Rec->getValueAsDef("dxil_version")->getValueAsInt("Minor");
       OS << "DXIL_OP_ATTRIBUTES(dxil::OpCode::" << Op.OpName << ", ";
       OS << std::to_string(Major) << ", " << std::to_string(Minor);
+      // These Attrs are the ones set for above DXIL version
       auto Attrs = Rec->getValueAsListOfDefs("fn_attrs");
+      // We will then iteratre through all possible attributes and mark the
+      // present ones as 'true' and all the others as 'false' to create the
+      // boolean table, eg) true, false, false, false
       for (const Record *Attr :
            Records.getAllDerivedDefinitions("DXILAttribute")) {
         std::string HasAttr = ", false";
@@ -408,7 +419,7 @@ static void emitDXILOpAttributes(const RecordKeeper &Records,
   OS << "#endif\n\n";
 }
 
-/// Emit a list of DXIL op properties and their query functions
+/// Emit a list of DXIL op properties
 static void emitDXILProperties(const RecordKeeper &Records, raw_ostream &OS) {
   OS << "#ifdef DXIL_PROPERTY\n";
   for (const Record *Prop : Records.getAllDerivedDefinitions("DXILProperty"))