[PATCH] D117006: [PowerPC] Add custom lowering for SELECT_CC fp128 using xsmaxcqp

[Qiu Chaofan via Phabricator via llvm-commits]

qiucf added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:1345
+    if (Subtarget.hasP10Vector()) {
+      setOperationAction(ISD::SELECT_CC, MVT::f128, Custom);
+    }
----------------
We already have a `setOperationAction(ISD::SELECT_CC, MVT::f128, Custom);` under `Subtarget.hasVSX()`, so don't need this.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:8033
+      else
+        return DAG.getNode(PPCISD::XSMINCDP, dl, Op.getValueType(), LHS, RHS);
     }
----------------
We don't need to add new ISD node, but `XS(MAX|MIN)CDP` node need renaming.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.td:224
+def PPCxsmaxcq : SDNode<"PPCISD::XSMAXCQP", SDT_PPCFPMinMax, []>;
+def PPCxsmincq : SDNode<"PPCISD::XSMINCQP", SDT_PPCFPMinMax, []>;
 def PPChi       : SDNode<"PPCISD::Hi", SDTIntBinOp, []>;
----------------
Operator `PPCxsmaxc` is type agnostic, we don't need to define `PPCxsmaxcq`.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrPrefix.td:1915
 let Predicates = [Power10Instrs] in {
-  def XSMAXCQP : X_VT5_VA5_VB5<63, 676, "xsmaxcqp", []>;
-  def XSMINCQP : X_VT5_VA5_VB5<63, 740, "xsmincqp", []>;
+  def XSMAXCQP : X_VT5_VA5_VB5<63, 676, "xsmaxcqp",
+                               [(set f128:$vT, (PPCxsmaxcq f128:$vA, f128:$vB))]>;
----------------
I'm not sure why `xscmpeqqp` are located here, but this should be better placed at `PPCInstrVSX.td`.


================
Comment at: llvm/test/CodeGen/PowerPC/scalar-min-max-p10.ll:1
+; RUN: llc -mcpu=pwr10 -ppc-asm-full-reg-names --enable-unsafe-fp-math \
+; RUN:   -verify-machineinstrs --enable-no-signed-zeros-fp-math \
----------------
Why not add them into `scalar-min-max.ll` (with a new `-mcpu=pwr10` runline)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117006