[PATCH] D115559: AMDGPU: Propagate amdgpu-waves-per-eu with attributor

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

LG, I left some notes and nits below.



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:676
 
-/// Propagate amdgpu-flat-work-group-size attribute.
-struct AAAMDFlatWorkGroupSize
+struct AAAMDSizeRangeAttribute
     : public StateWrapper<IntegerRangeState, AbstractAttribute, uint32_t> {
----------------
Docs: Base class to derive different size ranges.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:686-688
   /// See AbstractAttribute::getState(...).
   IntegerRangeState &getState() override { return *this; }
   const IntegerRangeState &getState() const override { return *this; }
----------------
Nit: I doubt you need these. StateWrapper should provide them, IIRC.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:691
+  /// See AbstractAttribute::trackStatistics()
+  void trackStatistics() const override {}
 
----------------
FWIW, we should be able to track how often we manifested, hence improved, the ranges. I think that would be good to have.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:709
       return true;
     };
 
----------------
Can you add a TODO in this class. The functionality it offers should be in some helper header. Effectively it does call site -> callee lookups and clamping. We have similar helpers in AttributorAttributes.cpp, but not this one. At some point we should move them all out into a header...


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:732
+    AttrList.push_back(Attribute::get(Ctx, AttrName, OS.str()));
     return IRAttributeManifest::manifestAttrs(A, getIRPosition(), AttrList,
                                               /* ForceReplace */ true);
----------------
Nit: You don't need a list with 8 slots, `{ Attr }` probably works just as well.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:814
+  bool isValidState() const override {
+    return !Assumed.isEmptySet() && IntegerRangeState::isValidState();
+  }
----------------
Nit: Probably better to only redirect one level up (`AAAMDSizeRangeAttribute::isValidState()`)

FWIW:
Empty should mean it's dead/misconfigured, no?


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

https://reviews.llvm.org/D115559