[PATCH] D66413: [ARM} Add support for MVE vmaxv

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMTargetTransformInfo.cpp:1064
     return false;
+  case Instruction::ICmp:
   case Instruction::Add:
----------------
samparker wrote:
> samtebbs wrote:
> > samparker wrote:
> > > samtebbs wrote:
> > > > samparker wrote:
> > > > > Is this actually needed for codegen?
> > > > It is, as otherwise the smax and umax vector reductions aren't generated and can't be selected on.
> > > As in, the reductions get expanded somewhere and your tests would fail?
> > Indeed, the tests fail and we get markedly worse codegen.
> > 
> > ```
> > 65c65,70
> > < 	vmaxv.u32	r2, q0
> > ---
> > > 	vmov.f32	s5, s3
> > > 	vmax.u32	q0, q0, q1
> > > 	vmov.32	r2, q0[1]
> > > 	vdup.32	q1, r2
> > > 	vmax.u32	q0, q0, q1
> > > 	vmov	r2, s0
> > ```
> Ok, cheers. Then I expect Dave is right that the vmin could now get generated and will then assert because it can't be selected...?
Yeah, I think there's two different places that these intrinsics can be expanded, once in a pre-isel pass if this returns false, or in ISEL if the instructions are expand.

So I think the selection would be OK, but it would be best to add vminv to make sure the vectoriser doesn't start generating them only for them the be messily expanded.


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

https://reviews.llvm.org/D66413