[PATCH] D15256: ARM: Better codegen for 64-bit compares.

[James Molloy via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.

Hi Peter,

Sorry for the round trip time on this, I've only just become aware of it.

Some comments but it generally looks fine. Thanks!

James


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:4719
@@ +4718,3 @@
+  SDValue CCR = DAG.getRegister(ARM::CPSR, MVT::i32);
+  SDValue Chain = DAG.getCopyToReg(DAG.getEntryNode(), DL, ARM::CPSR,
+                                   Cmp.getValue(1), SDValue());
----------------
This seems fishy to me. Shouldn't we be chaining to DAG.getRoot() (maybe?), rather than getEntryNode()? 

Surely we want CPSR's value just before the SETCCE, not at the start of the BB?

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:10502
@@ +10501,3 @@
+  if (CC == ARMCC::NE && LHS.getOpcode() == ISD::AND &&
+      isa<ConstantSDNode>(RHS) &&
+      cast<ConstantSDNode>(RHS)->getZExtValue() == 0 &&
----------------
This is very hard to read due to the many isa<> and casts<>. Can we please hoist these out as a dyn_cast to reduce code complexity?

================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:10578
@@ +10577,3 @@
+  if (CC == ARMCC::NE && LHS.getOpcode() == ARMISD::CMOV &&
+      isa<ConstantSDNode>(RHS) &&
+      cast<ConstantSDNode>(RHS)->getZExtValue() == 0 &&
----------------
Same here


http://reviews.llvm.org/D15256