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

Tue Jan 11 17:06:15 PST 2022

tingwang marked 2 inline comments as done.
tingwang added inline comments.

================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:1345
+    if (Subtarget.hasP10Vector()) {
+      setOperationAction(ISD::SELECT_CC, MVT::f128, Custom);
+    }
----------------
qiucf wrote:
> We already have a `setOperationAction(ISD::SELECT_CC, MVT::f128, Custom);` under `Subtarget.hasVSX()`, so don't need this.
That setOperationAction is located under the else part of Subtarget.hasP9Vector(), since P9Vector is part of P9InheritableFeatures, P10 test Subtarget.hasP9Vector() as true, so not hit that setOperationAction. Since this is supported by P10, and do not see Subtarget.hasP10Vector() predicate in this file, so added one under the Subtarget.hasAltivec() big block.

================
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))]>;
----------------
qiucf wrote:
> I'm not sure why `xscmpeqqp` are located here, but this should be better placed at `PPCInstrVSX.td`.
Thanks! I will move XS(MAX|MIN)CQP to sit together with XSMAXCDP.

================
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 \
----------------
qiucf wrote:
> Why not add them into `scalar-min-max.ll` (with a new `-mcpu=pwr10` runline)?
That test checks pwr8 behavior, which I don't like to do for fp128, and that's why added a separate file.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117006