[PATCH] D47332: [PowerPC] Exploit the vector min/max instructions

Tue Sep 18 07:02:52 PDT 2018

nemanjai planned changes to this revision.
nemanjai added a comment.

@jedilyn Hi Ke Wen, thanks for your comments. This needs some cleanup with regard to which instructions match the semantics of `F{MIN|MAX}NUM` vs. `F{MIN|MAX}NAN`. I'll clean that up and re-post this for your review. Thanks.

================
Comment at: lib/Target/PowerPC/PPCInstrAltivec.td:931
+          (v4f32 (VMINFP $src1, $src2))>;
+
 // Shuffles.
----------------
jedilyn wrote:
> Hi Nemanja, I have one concern on whether these two hardware instructions for vector float point can be perfectly mapped to these two ISDNode. 
> As the description of fmaxnum/fminnum //"in the case where a single input is NaN, the non-NaN input is returned.",// 
> while the description for the vmaxfp/vminfp in ISA like// "The maximum of +0 and -0 is +0. The maximum of any value and a NaN is a QNaN." //
> It looks more suitable for the fmaxnan/fminnan?
I am really sorry about such a long delay in responding to this. You are absolutely right. The correct nodes are `FMINNAN` and `FMAXNAN`. I think what I meant to use here are `XVMAXDP, XVMINDP, XVMAXSP, XVMINSP`. Those have the mentioned semantics (i.e. comparing a value and a NaN returns the value). And I don't think we need to worry about signalling NaNs at this time.

================
Comment at: lib/Target/PowerPC/PPCInstrVSX.td:1507
+            (v2i64 (VMINUD (COPY_TO_REGCLASS $src1, VRRC),
+                           (COPY_TO_REGCLASS $src2, VRRC)))>;
 } // AddedComplexity = 400
----------------
jedilyn wrote:
> I'm not sure why not use similar patterns like VMAXSW in PPCInstrAltivec.td but being located in HasP8Vector scope? Is there some special reasons with COPY_TO_REGCLASS? 
These instructions are new in ISA 2.07 so they have to be in the P8Vector block. Also, the `COPY_TO_REGCLASS` is needed because `VRRC` registers cannot contain `v2i64` values.

Repository:
  rL LLVM

https://reviews.llvm.org/D47332