[PATCH] D89831: [AArch64][SVE] Fix umin/umax lowering to handle out of range imm.

[Sander de Smalen via Phabricator via llvm-commits]

sdesmalen accepted this revision.
sdesmalen added a comment.
This revision is now accepted and ready to land.

LGTM, thanks for fixing @huihuiz!



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:3139
+    uint64_t ImmVal = CNode->getZExtValue();
     SDLoc DL(N);
+
----------------
nit: `DL` can be inlined directly into its single use, i.e.
```Imm = CurDAG->getTargetConstant(ImmVal, SDLoc(N), MVT::i32);```


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelDAGToDAG.cpp:3146
+    case MVT::i16:
+      ImmVal &= 0xFFFF;
+      break;
----------------
efriedma wrote:
> huihuiz wrote:
> > sdesmalen wrote:
> > > Maybe I'm missing something obvious, but I don't see why any masking is needed.
> > > Is removing `ImmVal = ImmVal & 0xFF;` not sufficient?
> > We need to apply masking "ImmVal & 0xFF" for i8 at least. Otherwise we got regression.
> > 
> > Take t.ll below, if we don't apply the mask for i8, then we generate "mov     z1.b, #-127 ".
> > With the mask we generate "umax    z0.b, z0.b, #129".
> > There are similar regression for i8 in range [128, 255]
> > 
> > ```
> > define <vscale x 16 x i8> @umax_i8_large(<vscale x 16 x i8> %a) {
> >   %elt = insertelement <vscale x 16 x i8> undef, i8 129, i32 0
> >   %splat = shufflevector <vscale x 16 x i8> %elt, <vscale x 16 x i8> undef, <vscale x 16 x i32> zeroinitializer
> >   %cmp = icmp ugt <vscale x 16 x i8> %a, %splat
> >   %res = select <vscale x 16 x i1> %cmp, <vscale x 16 x i8> %a, <vscale x 16 x i8> %splat
> >   ret <vscale x 16 x i8> %res
> > }
> > ```
> To add to this, the SelectionDAG node for the operand looks something like the following: `nxv16i8 AArch64ISD::DUP(i32 -127)`.
Thanks, I see what you mean now!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89831