[PATCH] D32245: Add an IR expansion pass for the experimental reductions

[Amara Emerson via Phabricator via llvm-commits]

aemerson marked 4 inline comments as done.
aemerson added a subscriber: RKSimon.
aemerson added inline comments.


================
Comment at: lib/CodeGen/ExpandReductions.cpp:82
+  for (inst_iterator I = inst_begin(F), E = inst_end(F); I != E; I = NextIt) {
+    NextIt = std::next(I);
+    auto II = dyn_cast<IntrinsicInst>(&*I);
----------------
mkuper wrote:
> I'm not a huge fan of this - I would prefer not to rely on the invalidation semantics. Maybe collect all relevant instructions into a vector first, then do the replacement?
> (But if others disagree, this is fine.)
I can do that, but I've seen this kind of thing done before in other places.


================
Comment at: lib/CodeGen/ExpandReductions.cpp:84
+    auto II = dyn_cast<IntrinsicInst>(&*I);
+    if (!II)
+      continue;
----------------
mkuper wrote:
> How do you expect this to happen?
Sorry, not entirely sure what you mean? This is an early exit if the given instruction isn't an intrinsic call.


================
Comment at: lib/CodeGen/ExpandReductions.cpp:92
+    case Intrinsic::experimental_vector_reduce_fmul:
+      // FMFs must be attached to the call, otherwise it's an ordered reduction
+      // and it can't be handled by generating this shuffle sequence.
----------------
mkuper wrote:
> What do we expect to happen in this case?
As no in-tree target currently supports ordered reductions, and given that for SVE we want to enable support completely without using this expansion pass, I decided against trying to handle ordered reductions here. We just skip the intrinsic if we find it's an ordered reduction.

If other targets want to experiment with ordered I think they can implement expansion via some scalarization method here.


================
Comment at: lib/CodeGen/ExpandReductions.cpp:117
+    case Intrinsic::experimental_vector_reduce_fmin: {
+      Vec = II->getArgOperand(0);
+      auto MRK = getMRK(ID);
----------------
mkuper wrote:
> I'd expect a target query somewhere, regarding whether the intrinsic needs to be expanded.
My expectation was that targets wouldn't need, at least at first, that level of granularity. @RKSimon what do you think about this?


Repository:
  rL LLVM

https://reviews.llvm.org/D32245