[PATCH] D87705: [PowerPC] Implement Set Boolean Condition Instructions

[Amy Kwan via Phabricator via llvm-commits]

amyk added inline comments.


================
Comment at: llvm/test/CodeGen/PowerPC/p10-spill-creq.ll:4
+; RUN:     -mcpu=pwr10 -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr < %s | \
+; RUN:     FileCheck %s
+
----------------
NeHuang wrote:
> Use `--check-prefix=CHECK-P10` here?
Since it's a P10 file, I will omit the `CHECK-P10` and just use the default `CHECK` instead in the other files, as per the suggestion.


================
Comment at: llvm/test/CodeGen/PowerPC/p10-spill-crgt.ll:4
+; RUN:     -mcpu=pwr10 -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr < %s | \
+; RUN:     FileCheck %s
+
----------------
NeHuang wrote:
> Use `--check-prefix=CHECK-P10` here?
Since it's a P10 file, I will omit the `CHECK-P10` and just use the default `CHECK` instead in the other files, as per the suggestion.


================
Comment at: llvm/test/CodeGen/PowerPC/p10-spill-crlt.ll:4
+; RUN:     -mcpu=pwr10 -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr < %s | \
+; RUN:     FileCheck %s
+
----------------
NeHuang wrote:
> Use `--check-prefix=CHECK-P10` here to be consistent with other IR cases
Since it's a P10 file, I will omit the `CHECK-P10` and just use the default `CHECK` instead in the other files, as per the suggestion.


================
Comment at: llvm/test/CodeGen/PowerPC/p10-spill-crun.ll:4
+; RUN:     -mcpu=pwr10 -ppc-asm-full-reg-names -ppc-vsr-nums-as-vr < %s | \
+; RUN:     FileCheck %s
+
----------------
NeHuang wrote:
> Use `--check-prefix=CHECK-P10` here to be consistent with other IR cases?
Since it's a P10 file, I will omit the `CHECK-P10` and just use the default `CHECK` instead in the other files, as per the suggestion.


================
Comment at: llvm/test/CodeGen/PowerPC/testComparesi32gtu.ll:72
+; CHECK-P10:         cmplw r4, r3
+; CHECK-P10-NEXT:    setbc r3, gt
 entry:
----------------
NeHuang wrote:
> Are we missing the instructions check between `setbcr3, gt` and blr?
Actually, the code is a bit different on LE/BE. 
On LE, the next line is actually `b fn2 at notoc`. 
On BE, there is other code, followed by `blr`. It will probably be good to show this. 


================
Comment at: llvm/test/CodeGen/PowerPC/testComparesi32leu.ll:15
+; RUN:   FileCheck %s --check-prefix=CHECK-P10 --implicit-check-not cmpw \
+; RUN:   --implicit-check-not cmpd --implicit-check-not cmpl
 
----------------
lei wrote:
> I am not sure what this line is verifying.  The `--implicit-check-not` identicates it's making sure that we are not generating p8/p9 set boolean instructions since it's checking for `cmpw` and not `cmplw` which is generated for P10.  However, the run line does not contain the option to turn on integer compare codegen ...
Yes, you're right. Those shouldn't be needed. I left them there by accident - they'll be removed in the updated diff. 


================
Comment at: llvm/test/CodeGen/PowerPC/testComparesi32ltu.ll:10
 ; RUN:  --check-prefixes=CHECK,LE
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64-unknown-linux-gnu -O2 \
+; RUN:     -ppc-asm-full-reg-names -mcpu=pwr10 < %s | \
----------------
lei wrote:
> Do we want to add and tests for integer compare codegen on P10?
That is a good question and may not be a bad idea. I think I can add them and if there are any objections to their addition, I can remove them. 


================
Comment at: llvm/test/CodeGen/PowerPC/testComparesi32ltu.ll:72
+; CHECK-P10:         cmplw r4, r3
+; CHECK-P10-NEXT:    setbc r3, lt
 entry:
----------------
NeHuang wrote:
> Are we missing the instructions check between `setbcr3, lt` and `blr`?
Will be updating the checks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87705