[PATCH] D85855: [SVE] Lower fixed length vector integer SMIN/SMAX

[Paul Walker via Phabricator via llvm-commits]

paulwalker-arm accepted this revision.
paulwalker-arm added a comment.

Thanks @cameron.mcinally, just a final typo to fix, plus a decision as to which version you'd rather land.



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:3624
+    bool OverrideNEON = VT == MVT::v2i64 || VT == MVT::v1i64;
+    return LowerToPredicatedOp(Op, DAG, AArch64ISD::SMAX_PRED, OverrideNEON);
+  }
----------------
cameron.mcinally wrote:
> Is this what you had in mind, @paulwalker-arm? 
Yes, although this and the previous version were both valid.  

For LowerMul I needed the explicit i64 check because the function is called for other element types, whereas in this instance you know only those types requested by this patch will be lowered.

If something weird happens the previous version would still work because it's safe to lower the i8, i16 and i32 vector element types to SVE, whereas the new version will likely crash and burn.

Personally I prefer the resilience of the previous version but since these are failure cases I doubt that matters plus an assert's build will show where somebody has gone wrong.


================
Comment at: llvm/test/CodeGen/AArch64/sve-fixed-length-int-minmax.ll:42
+  %res = call <16 x i8> @llvm.smax.v16i8(<16 x i8> %op1, <16 x i8> %op2)
+  ret <16 x i8> %res;
+}
----------------
Rouge ";"


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

https://reviews.llvm.org/D85855