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

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added reviewers: delena, rengolin.
mkuper added a comment.

Adding some target people - I think they ought to care about this more than I do. :-)



================
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);
----------------
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.)


================
Comment at: lib/CodeGen/ExpandReductions.cpp:84
+    auto II = dyn_cast<IntrinsicInst>(&*I);
+    if (!II)
+      continue;
----------------
How do you expect this to happen?


================
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.
----------------
What do we expect to happen in this case?


================
Comment at: lib/CodeGen/ExpandReductions.cpp:96
+        continue;
+      Vec = II->getArgOperand(1);
+    case Intrinsic::experimental_vector_reduce_add:
----------------
Please annotate the fallthrough here. (And perhaps it would be better to rewrite this to avoid it)


================
Comment at: lib/CodeGen/ExpandReductions.cpp:105
+      auto Rdx = getShuffleReduction(Builder, Vec, getOpcode(ID));
+      cast<Instruction>(Rdx)->setDebugLoc(II->getDebugLoc());
+      II->replaceAllUsesWith(Rdx);
----------------
What about the internal instructions?


================
Comment at: lib/CodeGen/ExpandReductions.cpp:117
+    case Intrinsic::experimental_vector_reduce_fmin: {
+      Vec = II->getArgOperand(0);
+      auto MRK = getMRK(ID);
----------------
I'd expect a target query somewhere, regarding whether the intrinsic needs to be expanded.


Repository:
  rL LLVM

https://reviews.llvm.org/D32245