[llvm-commits] [PATCH] Re: Solicit code review (change to CodeGen)

[Shuxin Yang]

Hi, Takumi:

     Thank you so much for the code review and testing. The attached is 
the new patch with trailing whitespaces deleted.

Thanks
Shuxin

On 10/14/12 12:54 AM, NAKAMURA Takumi wrote:
> Please prune trailing whitespaces.
>
> It passed selfhosting, for me.
> http://bb.pgr.jp/builders/clang-3stage-x86_64-linux/builds/657
> http://bb.pgr.jp/builders/clang-3stage-cygwin/builds/352
>
> ...Takumi
>
>
> 2012/10/13 Shuxin Yang <shuxin.llvm at gmail.com>:
>> Hi,
>>
>>      My previous change has a bug: I negated the condition code of a CMOV,
>> and go ahead creating a new CMOV using the *ORIGINAL* condition code.
>>
>>     This diff.patch passes the tests of "make check-all -C <buildroot>test",
>> the SingleSource and MultiSource testing under projects/test_suite/.
>>
>> Thanks
>>
>> Shuxin
>>
>>
>>
>> On 10/10/12 1:20 PM, Shuxin Yang wrote:
>>> Hi,
>>>
>>>    The attached is the fix to radar://11663049. The optimization can be
>>> outlined by following rules:
>>>
>>>      (select (x != c), e, c) -> select (x != c), e, x),
>>>      (select (x == c), c, e) -> select (x == c), x, e)
>>> where the <c> is an integer constant.
>>>
>>>    The reason for this change is that : on x86,
>>> conditional-move-from-constant needs two instructions;
>>> however, conditional-move-from-register need only one instruction.
>>>
>>>     While the LowerSELECT() sounds to be the most convenient place for this
>>> optimization, it turns out to be a bad place.The reason is that by replacing
>>> the constant <c> with a symbolic value, it obscure some
>>> instruction-combining opportunities which would otherwise be very easy to
>>> spot. For that reason, I have to postpone the change to last
>>> instruction-combining phase.
>>>
>>>     The change passes the test of "make check-all -C <build-root/test" and
>>> "make -C project/test-suite/SingleSource".
>>>
>>> Thanks
>>> Shuxin
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>

-------------- next part --------------
Index: test/CodeGen/X86/select_const.ll
===================================================================
--- test/CodeGen/X86/select_const.ll	(revision 0)
+++ test/CodeGen/X86/select_const.ll	(revision 0)
@@ -0,0 +1,16 @@
+; RUN: llc < %s -mtriple=x86_64-apple-darwin10 -mcpu=corei7 | FileCheck %s
+
+define i64 @test1(i64 %x) nounwind {
+entry:
+  %cmp = icmp eq i64 %x, 2
+  %add = add i64 %x, 1
+  %retval.0 = select i1 %cmp, i64 2, i64 %add
+  ret i64 %retval.0
+
+; CHECK: test1:
+; CHECK: leaq 1(%rdi), %rax
+; CHECK: cmpq $2, %rdi
+; CHECK: cmoveq %rdi, %rax
+; CHECK: ret
+
+}
Index: lib/Target/X86/X86ISelLowering.cpp
===================================================================
--- lib/Target/X86/X86ISelLowering.cpp	(revision 165952)
+++ lib/Target/X86/X86ISelLowering.cpp	(working copy)
@@ -14430,6 +14430,7 @@
       if (TrueC->getAPIntValue().ult(FalseC->getAPIntValue())) {
         CC = X86::GetOppositeBranchCondition(CC);
         std::swap(TrueC, FalseC);
+        std::swap(TrueOp, FalseOp);
       }
 
       // Optimize C ? 8 : 0 -> zext(setcc(C)) << 3.  Likewise for any pow2/0.
@@ -14512,6 +14513,46 @@
       }
     }
   }
+
+  // Handle these cases:
+  //   (select (x != c), e, c) -> select (x != c), e, x),
+  //   (select (x == c), c, e) -> select (x == c), x, e)
+  // where the c is an integer constant, and the "select" is the combination
+  // of CMOV and CMP.
+  //
+  // The rationale for this change is that the conditional-move from a constant
+  // needs two instructions, however, conditional-move from a register needs
+  // only one instruction.
+  //
+  // CAVEAT: By replacing a constant with a symbolic value, it may obscure
+  //  some instruction-combining opportunities. This opt needs to be
+  //  postponed as late as possible.
+  //
+  if (!DCI.isBeforeLegalize() && !DCI.isBeforeLegalizeOps()) {
+    // the DCI.xxxx conditions are provided to postpone the optimization as
+    // late as possible.
+
+    ConstantSDNode *CmpAgainst = 0;
+    if ((Cond.getOpcode() == X86ISD::CMP || Cond.getOpcode() == X86ISD::SUB) &&
+        (CmpAgainst = dyn_cast<ConstantSDNode>(Cond.getOperand(1))) &&
+        dyn_cast<ConstantSDNode>(Cond.getOperand(0)) == 0) {
+
+      if (CC == X86::COND_NE &&
+          CmpAgainst == dyn_cast<ConstantSDNode>(FalseOp)) {
+        CC = X86::GetOppositeBranchCondition(CC);
+        std::swap(TrueOp, FalseOp);
+      }
+
+      if (CC == X86::COND_E &&
+          CmpAgainst == dyn_cast<ConstantSDNode>(TrueOp)) {
+        SDValue Ops[] = { FalseOp, Cond.getOperand(0),
+                          DAG.getConstant(CC, MVT::i8), Cond };
+        return DAG.getNode(X86ISD::CMOV, DL, N->getVTList (), Ops,
+                           array_lengthof(Ops));
+      }
+    }
+  }
+
   return SDValue();
 }