[PATCH] D23614: [PPC] Generate positive FP zero using xor insn instead of loading from constant area

[Ehsan Amiri via llvm-commits]

amehsan added inline comments.

================
Comment at: lib/Target/PowerPC/PPCISelLowering.h:746
@@ -745,2 +745,3 @@
 
+    bool isFPImmLegal(const APFloat &Imm, EVT VT) const override;
   private:
----------------
nemanjai wrote:
> Perhaps a comment about what the use of this function is since this patch has none.
The name is clear. Isn't it?

================
Comment at: test/CodeGen/PowerPC/fast-isel-fcmp-nan.ll:5
@@ -4,3 +4,3 @@
 ; CHECK-LABEL: TestULT:
-; CHECK: mcrf
+; CHECK: xscmpudp
 ; CHECK: blr
----------------
nemanjai wrote:
> I don't understand the changes to this test case. How is the VSX floating point comparison replacing the move of a field within the condition register and why? I don't really understand the intent of the original test either, but nonetheless checking for these instructions does not seem like the equivalent test.
I was not able to understand the intention of this testcase. (That said, I didn't check git blame. There might be some info there).

The mcrf here, follows a fcmpu and copies BF 0 to BF7. the new VSX cmp insn, puts the result directly in BF 7. So that was the closest thing that I could have put here. I will check git blame, and if I found something useful to add, I will modify it. But with a testcase, whose intention is not known, I think this change should be fine.

================
Comment at: test/CodeGen/PowerPC/pzero-fp-xored.ll:1
@@ +1,2 @@
+; RUN: llc -mtriple=powerpc-unknown-linux-gnu -mattr=+vsx < %s | FileCheck %s
+; RUN: llc -mtriple=powerpc-unknown-linux-gnu -mattr=-vsx < %s | FileCheck %s --check-prefix=CHECK-NOT
----------------
nemanjai wrote:
> I think it might be a good idea to include `--implicit-check-not` on this line and specify which patterns (loading a zero from constant pool?) you want to not see.
Yes, I can add that.

================
Comment at: test/CodeGen/PowerPC/pzero-fp-xored.ll:2
@@ +1,3 @@
+; RUN: llc -mtriple=powerpc-unknown-linux-gnu -mattr=+vsx < %s | FileCheck %s
+; RUN: llc -mtriple=powerpc-unknown-linux-gnu -mattr=-vsx < %s | FileCheck %s --check-prefix=CHECK-NOT
+
----------------
nemanjai wrote:
> I don't think it's a particularly good idea to use a builtin check string (`CHECK-NOT`) as a check prefix. In fact, I'm not sure what the semantics of this actually are. And in any case, using different check prefixes for the two run lines should probably include equivalent checking (including CHECK-WHATEVER-PREFIX-LABEL:).
I had forgot that CHECK-NOT is built-in. Will change that.


https://reviews.llvm.org/D23614