[PATCH] D25221: [PPC][DAGCombine] Convert SETCC to subtract when the result is zero extended

[Ehsan Amiri via llvm-commits]

amehsan added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:9946
+  // Now the least significant bit carries the result of original comparison.
+  auto Shifted = DAG.getNode(ISD::SRL, DL, MVT::i64, SubNode,
+                             DAG.getConstant(Size - 1, DL, MVT::i32));
----------------
nemanjai wrote:
> I'm just curious why we don't see this shift in the test.
That should be an oversight. I will add this. I think I probably use CHECK-NEXT to make sure all the sequence is there.


================
Comment at: test/CodeGen/PowerPC/setcc-to-sub.ll:40
+; CHECK-LABEL: @test2
+; CHECK: rlwinm [[RES1:[0-9]*]]
+; CHECK: rlwinm [[RES2:[0-9]*]]
----------------
nemanjai wrote:
> I'm kind of thinking that this should be a slightly stronger test - to show that the operands are swapped on the sub. The base register for the two loads is known, so maybe that can be used.
Will look into it.


================
Comment at: test/CodeGen/PowerPC/setcc-to-sub.ll:42
+; CHECK: rlwinm [[RES2:[0-9]*]]
+; CHECK: sub 3, [[RES1]], [[RES2]]
+; CHECK: xori 3, 3, 1
----------------
nemanjai wrote:
> I don't think this is guaranteed to be R3 until the next instruction.
That is correct. Will change it. (Even though I hope our register allocation never makes that mistake :)


================
Comment at: test/CodeGen/PowerPC/setcc-to-sub.ll:47
+}
+
+!1 = !{!2, !2, i64 0}
----------------
nemanjai wrote:
> I think for completeness, you should include tests for the other two condition codes.
Sure . Will do.


https://reviews.llvm.org/D25221