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

[ChenZheng via Phabricator via llvm-commits]

shchenz added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:1345
+    if (Subtarget.hasP10Vector()) {
+      setOperationAction(ISD::SELECT_CC, MVT::f128, Custom);
+    }
----------------
tingwang wrote:
> shchenz wrote:
> > tingwang wrote:
> > > 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.
> > What if we set `Custom` for `SELECT_CC` for all targets `hasVSX()`? I don't see a reason why we can not set ` setOperationAction(ISD::SELECT_CC, MVT::f128, Custom);` on Power9. If so, we can just set `SELECT_CC` for type `f128` under `hasVSX()` instead of setting it two times for P10 and hasVSX()?
> Had a try doing Custom on Power9 to SELECT_CC, that path caused dump. Following is the path taken by fp128/Power9/SELECT_CC, and picks up PPC::XSCMPUQP:
> PPCDAGToDAGISel::Select
> -> case ISD::SELECT_CC
> -->   SelectCC()
> 
> Seems the expected code paths for Power9 and Power10 are different, then we still need a predicate to differentiate that?
OK, thanks for the explanation. I think it crashes because we don't handle `PPCISD::XSMAXC` and `PPCISD::XSMINC` for type f128  on Power9 in the legalizer.

However, I really think the logic for select_cc is kind of confusing on Power, we legalize it(set operation as custom) for Power8 and Power10, and we set it legal on Power9 and then do the selecting just before we select from td files. Maybe we can do some code refactor here later to make the logic be more clear. This should be out of this PR.


================
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 \
----------------
Better to generate check lines by using scripts like `./utils/update_llc_test_checks.py`.

And could you please also add a RUN line for AIX?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D117006