[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:210
+
+  bool parse(cl::Option &O, StringRef ArgName, StringRef Arg,
+             std::string &Value) {
----------------
jrbyrnes wrote:
> kerbowa wrote:
> > So this parser is just validating the input? Is there some way to avoid parsing twice?
> I can rethink the design a bit.  Would you prefer the CLI parser to create the PipelineOrderGroups (e.g. Pipeline used in adding edges)? I can probably implement this here using enum and array of constructors. Thanks for comments.
Seems like we could use a SchedGroup factory that just needs the parser to tell what the isMemberFn and size are. Creating and initializing two classes that do essentially the same thing seems not ideal, but I could be misunderstanding IGroupTableEntry.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUIGroupLP.cpp:254
+
 static cl::opt<Optional<unsigned>>
     VMEMGroupMaxSize("amdgpu-igrouplp-vmem-group-size", cl::init(None),
----------------
jrbyrnes wrote:
> kerbowa wrote:
> > Can these be removed now since the size is embedded into amdgpu-igrouplp-order?
> The idea was to default to the prior iteration's style if no amdgpu-igrouplp-order was specified, but I see how these options may be confusing. I'll remove them.
I mostly just want to avoid having so many cl options. An alternative is to make them "really hidden".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127994