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

[Justin Hibbits via Phabricator via llvm-commits]

jhibbits marked 3 inline comments as done.
jhibbits added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:3886
+  default:
+    llvm_unreachable("Undefined SPE Predicate for CC\n");
+    break;
----------------
jsji wrote:
> 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`
You're right.  I didn't check the full path below before the getPredicateForSetCCForSPE().  However, getPredicateForSetCCForSPE() is *only* to be used for floating point checks.  getPredicateForSetCC() doesn't take a type.  I guess I could add a type argument for getPredicateForCC().


================
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:
> 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.
Thanks.  I was unaware of that script, and should probably poke around more to see what else is there that I can make use of in the future.


================
Comment at: llvm/test/CodeGen/PowerPC/spe.ll:110
 ; CHECK-LABEL: test_fcmpugt
 ; CHECK: efscmpgt
 ; CHECK: blr
----------------
jsji wrote:
> 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.
Good point.  Originally, this test was just to make sure the instruction itself was generated.  The other tests I adapted because they were easy to add in the correctness of the condition check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69483