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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAttributor.cpp:680
+    ConstantRange Range(APInt(32, Min), APInt(32, Max + 1));
+    intersectKnown(Range);
+  }
----------------
arsenm wrote:
> jdoerfert wrote:
> > Somewhat surprising you intersect the known range with an assumed result, if that is on purpose probably worth a comment explaining why this is reasonable.
> I'm confused by your surprise. This is the initialize, so the assumed state doesn't mean anything? All the other IntegerRangeState attributes start out with intersectKnown in initialize()
So, at this point you move something assumed by another AA into something known by this. However, we don't know the other AAs assumed state is valid yet. Only after the fixpoint is reached it is known to hold. So what other AAs do (I hope) is use IR information to setup the known state. Here we might end up assuming the best for the FlatWorkGroupSize (line 674) then making it to something known in 683, before the values are tightened during the fixpoint iteration causing the known set to be not actually valid anymore. Generally, assumed information should only flow into known information if a fixpoint is reached (at which point the assumed information becomes the known one anyway). Before that, assumed should flow into assumed only and known can flow in either.

Does that make some sense?


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

https://reviews.llvm.org/D115559