[PATCH] D99509: [RISCV] Add legality check for vectoring reduction

[Luke via Phabricator via llvm-commits]

luke957 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h:133
+    if (!VF.isScalable())
+      return true;
+
----------------
dmgreen wrote:
> craig.topper wrote:
> > luke957 wrote:
> > > craig.topper wrote:
> > > > This is returning true for not scalable. Is that saying that any fixed length reduction is supported?
> > > I understand returning true for not scalable is just to make canVectorizeReductions() in  LoopVectorizer.cpp return right value. There will be other checks after canVectorizeReductions() returns.
> > Let me rephrase a little. You're returning true for fixed vectors, but not checking element type or opcode or hasStdExtV. Does the the mean the vectorizer will start generating reductions for vectors of i128. Or reductions of floats when the F extension isn't enabled?
> I believe the "legal" in this function really means "are you going to crash if there is a reduction of this type". Normal non-scalable reductions can always be legalized to something, even if that mean expanding or scalarizing or converting to soft float.
> 
> The standard costmodelling then kicks in to say whether it is actually a good idea. i.e work the same as X86/Arm/etc. (But, because they are outside the loop, the vectorizer doesn't account for them directly, only the arithmetic instructions that will be in the loop. The assumption is what is in the loop will dominate performance).
I think so. But as StdExtV is optional for riscv, hasStdExtV() check might still be needed.


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h:151
+    case RecurKind::UMax:
+    case RecurKind::FMin:
+    case RecurKind::FMax:
----------------
craig.topper wrote:
> luke957 wrote:
> > frasercrmck wrote:
> > > We don't support the fmin/fmax reductions yet. I suspect this would crash in the backend?
> > The added test case contains a fmin case. For opt -loop-vectorize, it seems no crash.
> The crash would be in llc in SelectionDAG, not opt.
Silly of me. Add fixme for fmin/fmax.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99509