[PATCH] D127994: [AMDGPU] Expose CLI controls for IGroup ordering

[Austin Kerbow via Phabricator via llvm-commits]

kerbowa added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:52
+  CanAddMIFn canAddMI;
+  SmallVector<unsigned, 4> SubGroups;
+  Optional<unsigned> MaxSize;
----------------
Can you comment that this is a map from IGroupClass and just use an enum class?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:103
+  igp_valu,
+  igp_salu,
+  igp_vmem,
----------------
These enum declarations don't follow style conventions https://llvm.org/docs/CodingStandards.html


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:161
+    assert(Group < sizeof(ObservedGroups) * 8);
+    if (ObservedGroups & (1 << (unsigned)Group)) {
+      O.error("Multiple occurance of " + Token);
----------------
As above, should use a bitmask enum so the mappings are more explicit.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:210
+
+  bool parse(cl::Option &O, StringRef ArgName, StringRef Arg,
+             std::string &Value) {
----------------
So this parser is just validating the input? Is there some way to avoid parsing twice?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:254
+
 static cl::opt<Optional<unsigned>>
     VMEMGroupMaxSize("amdgpu-igrouplp-vmem-group-size", cl::init(None),
----------------
Can these be removed now since the size is embedded into amdgpu-igrouplp-order?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:460
+      // Grab the relevant IGroup Data.
+      // auto Match = std::find_if(IGroupTableEntries.begin(),
+      // IGroupTableEntries.end(),
----------------
Remove commented code.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:463
+      //  [&it](IGroupTableEntry &IGPData) {return IGPData.GroupName == *it;});
+      auto Match = IGroupMasterTable[*it];
+      assert(Match);
----------------
Can we record the order in the enums somehow instead of re-parsing the strings?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:100
+// Enum holding the supported IGroups
+enum IGroupClass {
+  igp_mfma,
----------------
Can this just reuse/replace SchedBarrierMasks?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127994/new/

https://reviews.llvm.org/D127994