[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:135
+
+    if (!VF.isScalable())
+      return true;
----------------
frasercrmck wrote:
> craig.topper wrote:
> > craig.topper wrote:
> > > frasercrmck wrote:
> > > > I'm wondering if returning `true`  for fixed-length vectors, even if correct (i.e. not crashing), is likely to produce worse code. Will it trick the cost modeling into producing code which we'll then expand during legalization, and increase register pressure and the likelihood of spilling?
> > > For most reductions the expansion should be log2(elements) SingleSrcPermute shuffles and binops to reduce elements by half each step. So it shouldn't increase the register pressure much since you just need 2 registers. This should match the default cost model for getArithmeticReductionCost/getMinMaxReductionCost.
> > > 
> > > That isn't what happens for ordered floating point reduction though, but it also doesn't look like this function is told that it is an ordered reduction. It doesn't look like getArithmeticReductionCost get's told either. Maybe we only vectorize to unordered?
> > I think the ordered propery is in RecurrenceDescriptor after https://reviews.llvm.org/D98435. But getArithmeticReductionCost/getMinMaxReductionCost don't receive the RecurrenceDescriptor.
> Yeah fair enough, that sounds fine then. Part of me also finds it weird that we'd say it's legal to generate a fixed-length fmin/fmax/mul reduction even though we'd expand it 100% of the time. But perhaps it's just that this API's job isn't perfectly clear.
The ordered propery is added for RecurrenceDescriptor in https://reviews.llvm.org/D98435 while in-order reduction support is off by default and controlled with the `-enable-strict-reductions` flag.   Would ordered floating point reduction be fixed in function InnerLoopVectorizer::fixVectorizedLoop() if `-enable-strict-reductions` flag is used?


================
Comment at: llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h:135
+
+    if (!VF.isScalable())
+      return true;
----------------
luke957 wrote:
> frasercrmck wrote:
> > craig.topper wrote:
> > > craig.topper wrote:
> > > > frasercrmck wrote:
> > > > > I'm wondering if returning `true`  for fixed-length vectors, even if correct (i.e. not crashing), is likely to produce worse code. Will it trick the cost modeling into producing code which we'll then expand during legalization, and increase register pressure and the likelihood of spilling?
> > > > For most reductions the expansion should be log2(elements) SingleSrcPermute shuffles and binops to reduce elements by half each step. So it shouldn't increase the register pressure much since you just need 2 registers. This should match the default cost model for getArithmeticReductionCost/getMinMaxReductionCost.
> > > > 
> > > > That isn't what happens for ordered floating point reduction though, but it also doesn't look like this function is told that it is an ordered reduction. It doesn't look like getArithmeticReductionCost get's told either. Maybe we only vectorize to unordered?
> > > I think the ordered propery is in RecurrenceDescriptor after https://reviews.llvm.org/D98435. But getArithmeticReductionCost/getMinMaxReductionCost don't receive the RecurrenceDescriptor.
> > Yeah fair enough, that sounds fine then. Part of me also finds it weird that we'd say it's legal to generate a fixed-length fmin/fmax/mul reduction even though we'd expand it 100% of the time. But perhaps it's just that this API's job isn't perfectly clear.
> The ordered propery is added for RecurrenceDescriptor in https://reviews.llvm.org/D98435 while in-order reduction support is off by default and controlled with the `-enable-strict-reductions` flag.   Would ordered floating point reduction be fixed in function InnerLoopVectorizer::fixVectorizedLoop() if `-enable-strict-reductions` flag is used?
I agree the API's name is weird.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99509