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

[Nemanja Ivanovic via llvm-commits]

nemanjai 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:
----------------
Perhaps a comment about what the use of this function is since this patch has none.

================
Comment at: test/CodeGen/PowerPC/fast-isel-fcmp-nan.ll:5
@@ -4,3 +4,3 @@
 ; CHECK-LABEL: TestULT:
-; CHECK: mcrf
+; CHECK: xscmpudp
 ; CHECK: blr
----------------
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.

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

================
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
+
----------------
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:).


https://reviews.llvm.org/D23614