[PATCH] D130869: [AMDGPU] Add GCNMaxILPSchedStrategy

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/GCNSchedStrategy.h:32
+  PreRARematerialize = 3,
+  ILPInitialSchedule = 4,
+  LastStage = ILPInitialSchedule
----------------
kerbowa wrote:
> rampitec wrote:
> > AFAIR PreRARematerialize shall be a last stage, it was leaving some variables in an inconsistent state. Before the last refactoring there was even static_assert about that.
> > 
> > I see that you are building stages pipeline within SchedStages, but probably it makes sense to reorder the enum and redefine operator++ to walk SchedStages instead of static casting integers now.
> The idea is that different SchedStrategies may have different stages or permutations of stages.
> 
> I already defined operator++ in a previous patch. I'm not sure what casting of integers you are referring to.
> 
> In this patch, each SchedStrategy has the order of its stages defined in the SchedStages vector. I should make the max occupancy strategy assert that PreRARemat is the last stage in that vector and make all the checks relative to that vector.
I mean the cast at line 42. It might be clearer to use a next stage from SchedStages vector rather than just incrementing GCNSchedStageID numerically. Ten years from now nobody will remember why PreRARematerialize shall be a last one, and code can easily avoid using that operator alltogether, like it does using a range based loop for SchedStages in the runSchedStages. Then imagine you would like to add it to the ILP pipeline too, it will just break operator++ logic.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130869