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

Mon Dec 30 13:48:14 PST 2019

jhibbits marked 4 inline comments as done.
jhibbits added a comment.

In D69483#1799302 <https://reviews.llvm.org/D69483#1799302>, @jsji wrote:

> Looks like to me that Vector Compare in SPE also has different CR bit semantics . 
>  So this patch will only handle floating point part for SPE? If so, maybe we should make it explicit in title.

Correct.  We currently only do codegen for SPE floating point, not for vector.  I currently have no plans to implement vector support any time soon, if ever.

================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:3886
+  default:
+    llvm_unreachable("Undefined SPE Predicate for CC\n");
+    break;
----------------
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.

================
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
----------------
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.

================
Comment at: llvm/test/CodeGen/PowerPC/spe.ll:1
 ; RUN: llc -verify-machineinstrs < %s -mtriple=powerpc-unknown-linux-gnu \
 ; RUN:          -mattr=+spe |  FileCheck %s
----------------
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.

================
Comment at: llvm/test/CodeGen/PowerPC/spe.ll:110
 ; CHECK-LABEL: test_fcmpugt
 ; CHECK: efscmpgt
 ; CHECK: blr
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69483