[PATCH] D13121: Improve ISel across lane float min/max reduction

[James Molloy via llvm-commits]

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

Hi Jun,

This looks fine, if you address my last comment.

Thanks,

James


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:8601
@@ -8600,3 +8600,3 @@
   int NumVecElts = VTy.getVectorNumElements();
-  if (NumVecElts != 4 && NumVecElts != 8 && NumVecElts != 16)
-    return SDValue();
+  if (Op == ISD::FMAXNUM || Op == ISD::FMINNUM) {
+    if (NumVecElts != 4)
----------------
junbuml wrote:
> jmolloy wrote:
> > What about FMAXNAN and FMINNAN (-> FMAXV, FMINV)?
> I also thought the same, but I wasn't able to generate FMAXNAN with a vector input. It appears that matchSelectPattern() cannot return SPNB_RETURNS_NAN with fcmp fast. And even without fast math flag, SPNB_RETURNS_NAN cannot be returned because both LHSSafe and RHSSafe in matchSelectPattern() are false for vector inputs. Please let me know if I miss something.
>  
>  
Yes, it does appear you're right. I'll have to go fix that.


================
Comment at: lib/Target/AArch64/AArch64ISelLowering.cpp:8662
@@ -8656,3 +8661,3 @@
   case ISD::ADD:
-    Opcode = AArch64ISD::UADDV;
+    Opcode = Intrinsic::aarch64_neon_uaddv;
     break;
----------------
junbuml wrote:
> jmolloy wrote:
> > Why are you making this change? What's the rationale?
> The only reason that I change to use intrinsics here is just to be consistent in the way I handle nodes because I could see only intrinsic of FMAXNMV, no SDNode for FMAXNMV. If SDNode need to be used I will add SDNode definition in td file for  FMAXNMV. Please let me know.
>  
Given that we have these cross-lane nodes, it makes sense to use them in preference to the intrinsic version.

As there are no such nodes for FMAXNMV and friends, the intrinsics make sense. I don't think there is any need to create ISDNodes for FMAXNMV and friends.


http://reviews.llvm.org/D13121