[llvm-commits] [PATCH] add-carray/sub-borrow optimization (CodeGen, X86)

[manman ren]

On Oct 31, 2012, at 10:30 AM, Shuxin Yang <shuxin.llvm at gmail.com> wrote:

> Hi, Manman:
> 
>   I read "coding guide" the other day. The document also suggests "return early" to make the logic easier.
> But it doesn't say on what condition the "return early" is appropriate. In this case, we have to duplicate
> non-trivial code in order to return early.  For sloppy engineers like me, maintaining duplication is difficult.
> I'd like to improve maintenance even at the cost of sacrificed readability.

We can probably get rid of 
>>>> +      SDValue NewVal = DAG.getNode(X86ISD::SETCC, DL, N->getVTList(),
>>>> +                                   DAG.getConstant(CC, MVT::i8), EFLAGS);
>>>> +      N = NewVal.getNode ();
since SETCC will be replaced with a SETCC_CARRAY and N is not used in the followed if statement.

Otherwise LGTM.

Thanks,

> 
> Thanks
> Shuxin
>>>> +      CC = X86::COND_B;
>>>> +      SDValue NewSub = DAG.getNode(X86ISD::SUB, EFLAGS.getDebugLoc(),
>>>> +                                   EFLAGS.getNode()->getVTList(),
>>>> +                                   EFLAGS.getOperand(1), EFLAGS.getOperand(0));
>>>> +      EFLAGS = SDValue(NewSub.getNode(), EFLAGS.getResNo());
>>>> +      SDValue NewVal = DAG.getNode(X86ISD::SETCC, DL, N->getVTList(),
>>>> +                                   DAG.getConstant(CC, MVT::i8), EFLAGS);
>>>> +      N = NewVal.getNode ();
>> 
>> I suggested to return instead of falling to the "if" mainly due to readability.
>>   if (CC == X86::COND_B)
>>     return DAG.getNode(ISD::AND, DL, MVT::i8,
>>                        DAG.getNode(X86ISD::SETCC_CARRY, DL, MVT::i8,
>>                                    DAG.getConstant(CC, MVT::i8), EFLAGS),
>>                        DAG.getConstant(1, MVT::i8));
>> 
>> If we return SETCC_CARRY directly, there is no need to create NewVal.
>> I guess we have different opinions on readability :)
>> 
>> Thanks,
>> Manman
>> 
>> 
>