<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-07 8:29 GMT+04:00 Nadav Rotem <span dir="ltr"><<a href="mailto:nrotem@apple.com" target="_blank">nrotem@apple.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div>Can you please make the check (Subtarget->slowIncDec()) the innermost check, after the code that checks for the immediate?</div></div></blockquote><div>Done. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><br></div><div><div>def : Pat<(add GR8 :$src, 1),</div><div>+ (INC8r GR8 :$src)>, Requires<[NotSlowIncDec]>;</div></div><div><br></div><div>The indentation of the pattern looks funny. Does it match the rest of the file ?</div>
</div></blockquote><div>I corrected indentation to match it to the rest of file.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><br></div><div>Everything else LGTM. </div></div></blockquote><div>Thank you. Committed as revision 210466. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div style="word-wrap:break-word">
<div><br></div><div>Thanks,</div><div>Nadav</div><div><br></div><br><div><div><div><div>On Jun 6, 2014, at 4:12 AM, Alexey Volkov <<a href="mailto:avolkov.intel@gmail.com" target="_blank">avolkov.intel@gmail.com</a>> wrote:</div>
<br></div></div><blockquote type="cite"><div><div><div dir="ltr">I updated patch according to your comments.</div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-05 20:32 GMT+04:00 Nadav Rotem <span dir="ltr"><<a href="mailto:nrotem@apple.com" target="_blank">nrotem@apple.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div><div>
On Jun 5, 2014, at 9:01 AM, Alexey Volkov <<a href="mailto:avolkov.intel@gmail.com" target="_blank">avolkov.intel@gmail.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr">Hi Nadav,<br><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-05 9:33 GMT+04:00 Nadav Rotem <span dir="ltr"><<a href="mailto:nrotem@apple.com" target="_blank">nrotem@apple.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Hi Alexey,<br>
<br>
Can you please reduce the size of the testcase? It is really big.<br></blockquote><div> I attached patch with reduced testcase.</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
<br>
- if (ConstantSDNode *C =<br>
- dyn_cast<ConstantSDNode>(ArithOp.getNode()->getOperand(1))) {<br>
- // An add of one will be selected as an INC.<br>
- if (C->getAPIntValue() == 1) {<br>
- Opcode = X86ISD::INC;<br>
- NumOperands = 1;<br>
- break;<br>
- }<br>
+ if (!Subtarget->slowIncDec())<br>
+ if (ConstantSDNode *C =<br>
+ dyn_cast<ConstantSDNode>(ArithOp.getNode()->getOperand(1))) {<br>
+ // An add of one will be selected as an INC.<br>
+ if (C->getAPIntValue() == 1) {<br>
+ Opcode = X86ISD::INC;<br>
+ NumOperands = 1;<br>
+ break;<br>
+ }<br>
<br>
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.<br></blockquote><div>Checking slowIncDec couldn't be placed in condition with ConstantSDNode *C since it's a declaration statement inside if().</div>
<div>Also slowIncDec field check should be cheaper than dyn_cast<> in if().</div></div></div></div></blockquote><div><br></div></div></div><div>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. </div>
<div><br><blockquote type="cite"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Will this work?<br>
-def : Pat<(add GR16:$src, -1), (DEC16r GR16:$src)>, Requires<[Not64BitMode, NotSlowIncDec]>;<br></blockquote><div>Yes, this will work. But I intentionally made changes with "let Predicates =" to get code more readable and structured. </div>
</div></div></div></blockquote><div><br></div></div><div>I think that adding another parameter and a comment above makes things more readable. </div><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Everything else looks okay.<br>
<br>
Thanks,<br>
Nadav<br>
<br>
<br>
On Jun 2, 2014, at 7:28 AM, Alexey Volkov <<a href="mailto:avolkov.intel@gmail.com" target="_blank">avolkov.intel@gmail.com</a>> wrote:<br>
<br>
> <D3990.10015.patch><br>
<br>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr">Alexey Volkov<div>Intel Corporation</div></div>
</div></div>
</div><span><incdec.patch></span></blockquote></div><br></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr">Alexey Volkov<div>Intel Corporation</div></div>
</div>
</div></div><span><incdec.patch></span></blockquote></div><br></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div dir="ltr">Alexey Volkov<div>Intel Corporation</div></div>
</div></div>