[PATCH] [X86] Use ADD/SUB instead of INC/DEC for Silvermont

[Alexey Volkov]

2014-06-07 8:29 GMT+04:00 Nadav Rotem <nrotem at apple.com>:

> Can you please make the check (Subtarget->slowIncDec()) the innermost
> check, after the code that checks for the immediate?
>
Done.

>
> def : Pat<(add GR8 :$src, 1),
> +          (INC8r     GR8 :$src)>, Requires<[NotSlowIncDec]>;
>
> The indentation of the pattern looks funny. Does it match the rest of the
> file ?
>
I corrected indentation to match it to the rest of file.

>
> Everything else LGTM.
>
Thank you. Committed as revision 210466.

>
> Thanks,
> Nadav
>
>
> On Jun 6, 2014, at 4:12 AM, Alexey Volkov <avolkov.intel at gmail.com> wrote:
>
> I updated patch according to your comments.
>
>
> 2014-06-05 20:32 GMT+04:00 Nadav Rotem <nrotem at apple.com>:
>
>>
>> On Jun 5, 2014, at 9:01 AM, Alexey Volkov <avolkov.intel at gmail.com>
>> wrote:
>>
>> Hi Nadav,
>>
>>
>> 2014-06-05 9:33 GMT+04:00 Nadav Rotem <nrotem at apple.com>:
>>
>>> Hi Alexey,
>>>
>>> Can you please reduce the size of the testcase? It is really big.
>>>
>>  I attached patch with reduced testcase.
>>
>>>
>>>
>>> -    if (ConstantSDNode *C =
>>> -        dyn_cast<ConstantSDNode>(ArithOp.getNode()->getOperand(1))) {
>>> -      // An add of one will be selected as an INC.
>>> -      if (C->getAPIntValue() == 1) {
>>> -        Opcode = X86ISD::INC;
>>> -        NumOperands = 1;
>>> -        break;
>>> -      }
>>> +    if (!Subtarget->slowIncDec())
>>> +      if (ConstantSDNode *C =
>>> +          dyn_cast<ConstantSDNode>(ArithOp.getNode()->getOperand(1))) {
>>> +        // An add of one will be selected as an INC.
>>> +        if (C->getAPIntValue() == 1) {
>>> +          Opcode = X86ISD::INC;
>>> +          NumOperands = 1;
>>> +          break;
>>> +        }
>>>
>>> Can you please perform the slowIncDec check after the ‘&&’ operator
>>> inside the second if?  This will help reducing the compile time and allow
>>> you to change less code.
>>>
>> Checking slowIncDec couldn't be placed in condition with ConstantSDNode
>> *C since it's a declaration statement inside if().
>> Also slowIncDec field check should be cheaper than dyn_cast<> in if().
>>
>>
>> So add another if inside.  I prefer that we change as little code as
>> possible. Also, almost no x86 target will satisfy slowIncDec so this check
>> will just make everybody else slower for no good reason. Just place the IF
>> inside.
>>
>>
>>> Will this work?
>>> -def : Pat<(add GR16:$src, -1), (DEC16r    GR16:$src)>,
>>> Requires<[Not64BitMode, NotSlowIncDec]>;
>>>
>> Yes, this will work. But I intentionally made changes with "let
>> Predicates =" to get code more readable and structured.
>>
>>
>> I think that adding another parameter and a comment above makes things
>> more readable.
>>
>>
>>> Everything else looks okay.
>>>
>>> Thanks,
>>> Nadav
>>>
>>>
>>> On Jun 2, 2014, at 7:28 AM, Alexey Volkov <avolkov.intel at gmail.com>
>>> wrote:
>>>
>>> > <D3990.10015.patch>
>>>
>>>
>>
>>
>> --
>> Alexey Volkov
>> Intel Corporation
>>  <incdec.patch>
>>
>>
>>
>
>
> --
> Alexey Volkov
> Intel Corporation
>  <incdec.patch>
>
>
>


-- 
Alexey Volkov
Intel Corporation
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140609/e5e9a74c/attachment.html>