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

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper 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);
----------------
aemerson wrote:
> 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.
I'd suggest getting another reviewer's opinion on this.


================
Comment at: lib/CodeGen/ExpandReductions.cpp:84
+    auto II = dyn_cast<IntrinsicInst>(&*I);
+    if (!II)
+      continue;
----------------
aemerson wrote:
> 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.
Sorry, I misread this is dyn_cast<Instruction>, ignore.


================
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.
----------------
aemerson wrote:
> 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.
The issue is that target-independent intrinsics are, by definition, supposed to be handled by any target. I shouldn't see a backend crash if I write IR that has the ordered intrinsic, and try to compile it for x86. 

Having said that - this is fine for now, but if we ever want to make these intrinsics non-experimental, this will have to be dealt with somehow. Please add a TODO.


================
Comment at: lib/CodeGen/ExpandReductions.cpp:117
+    case Intrinsic::experimental_vector_reduce_fmin: {
+      Vec = II->getArgOperand(0);
+      auto MRK = getMRK(ID);
----------------
aemerson wrote:
> RKSimon wrote:
> > aemerson wrote:
> > > 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?
> > Are we guaranteeing that the reductions will match the ones supported by TargetTransformInfo::getReductionCost ?
> Do you mean useReductionIntrinsic()? If so, I suppose it comes down to the exact use case of this expansion. Michael originally asked for this so that targets could check the effect of using the intrinsics at the IR level only, and at a very late stage converting them into the shuffle form we have now. For that, I don't see why you would care about which individual intrinsics are expanded, rather than a simple on/off decision.
> 
> If however there might be more uses of this, for example in future, if we want to enable intrinsic forms for *all* targets as a canonical form, and then use this pass with TTI to make a target dependent decision on which codegen-level form is preferred, then I think a TTI hook would make sense.
> 
> I can add a hook anyway, perhaps defaulting to "expand all intrinsics" unless the target overrides it.
> I can add a hook anyway, perhaps defaulting to "expand all intrinsics" unless the target overrides it.
That's exactly what I'd expect, thanks.


Repository:
  rL LLVM

https://reviews.llvm.org/D32245