[PATCH] D69483: [PowerPC]: Fix predicate handling with SPE

[Jinsong Ji via Phabricator via llvm-commits]

jsji added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:3886
+  default:
+    llvm_unreachable("Undefined SPE Predicate for CC\n");
+    break;
----------------
jhibbits wrote:
> jsji wrote:
> > If we don't merge this into `getPredicateForSetCC`, maybe it would be better to list those should have been `legalized` ones like `getPredicateForSetCC`.
> Since this is called in more places than getPredicateForSetCC, this comment makes more sense to implement.
? Why more places than `getPredicateForSetCC`? I think we call it after each  `getPredicateForSetCC`


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:5089
+    if (PPCSubTarget->hasSPE() &&
+        N->getOperand(2).getValueType().isFloatingPoint()) {
+      // For SPE instructions the result is in GT bit of the CR
----------------
jhibbits wrote:
> jsji wrote:
> > Why we are using `isFloatingPoint()` here, while checking `MVT::f32/MVT::f64` above?
> No real reason why.  I don't see any difference between them in this use case, but I can unify the conditions.
Yes, please.


================
Comment at: llvm/test/CodeGen/PowerPC/spe.ll:1
 ; RUN: llc -verify-machineinstrs < %s -mtriple=powerpc-unknown-linux-gnu \
 ; RUN:          -mattr=+spe |  FileCheck %s
----------------
jhibbits wrote:
> jsji wrote:
> > jsji wrote:
> > > Maybe we should include one RUN line for `-O0` to test FastIsel as well?
> > Why not generate the checks using script?
> Is there an example of generating checks via a script?  These checks were generated from a C file provided by @kthomsen a while back, with -emit-llvm, and pared down to the smallest working set I could.
Just run `llvm/utils/update_llc_test_checks.py --llc-binary ../build/bin/llc llvm/test/CodeGen/PowerPC/spe.ll` .
The script should generate CHECKs automatically.
We can then examine each of them to see whether they are desired.


================
Comment at: llvm/test/CodeGen/PowerPC/spe.ll:110
 ; CHECK-LABEL: test_fcmpugt
 ; CHECK: efscmpgt
 ; CHECK: blr
----------------
jhibbits wrote:
> jsji wrote:
> > Why don't we check bits with `ugt`?
> The asm generated with ugt is much larger, including extra efscmpeq checks as well, so I chose not to include that.
But then we lose the test point here?
If we can use `llvm/utils/update_llc_test_checks.py` to generate the test, then it should be easier to keep them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69483