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

Nadav Rotem nrotem at apple.com
Fri Jun 6 21:29:15 PDT 2014


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

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 ?

Everything else LGTM. 

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>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140606/6a461539/attachment.html>


More information about the llvm-commits mailing list