<div dir="ltr">Whoops, the autocorrection got me :P<div><br></div><div>What I meant is: if somebody with commit access agrees with this patch can commit it please? :)</div><div><br></div><div>Thanks,</div><div>Marcello</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-04 0:46 GMT-07:00 Marcello Maggioni <span dir="ltr"><<a href="mailto:hayarms@gmail.com" target="_blank">hayarms@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div dir="ltr">Thank you very much for your comments Daniil !<div><br></div><div>If anybody that commit access agrees also with these patches can somebody commit them please? :)</div><div><br></div><div>Cheers,</div><div>
Marcello</div>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-04 0:35 GMT-07:00 Daniil Troshkov <span dir="ltr"><<a href="mailto:troshkovdanil@gmail.com" target="_blank">troshkovdanil@gmail.com</a>></span>:<div>
<div class="h5"><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">ok</div><div><div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jun 4, 2014 at 10:30 AM, Marcello Maggioni <span dir="ltr"><<a href="mailto:hayarms@gmail.com" target="_blank">hayarms@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Here is an updated patch with the cases grouped in a macro.<div><br></div><div>I made the syntax of the macro such that it looks like when put in a switch it looks like a regular switch case.</div>
<div><br>
</div><div>I couldn't find a better way to address this sadly :/</div><div><br></div><div>The optimization patch remained the same, but I updated the version number.</div><div><br></div><div>Marcello</div></div><div class="gmail_extra">
<br><br><div class="gmail_quote">2014-06-03 8:04 GMT-07:00 Marcello Maggioni <span dir="ltr"><<a href="mailto:hayarms@gmail.com" target="_blank">hayarms@gmail.com</a>></span>:<div><div><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid">
Thanks. The macro could be an idea for that, yeah. I'll try to see if there is an alternative solution for this and in case I cannot find one I will implement your suggestion.<div><br></div><div>About the differences:</div>
<div><br></div><div>A and B should be the same (probably the order of the labels is different, but they are the same) and these represent the nodes that have binop flags attached to them.</div><div><br></div><div>In the optimisation code the switch cases are different because I was targeting a specific case identified by those instructions. (A subset of the nodes that have flags)</div>
<div><br></div><div>Marcello<br><br>Il martedì 3 giugno 2014, Daniil Troshkov <<a href="mailto:troshkovdanil@gmail.com" target="_blank">troshkovdanil@gmail.com</a>> ha scritto:<div><div><br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid">
<div dir="ltr"><div>1) Ok</div><div>2) Ok</div><div>3) I mean:</div><div> </div><div>A)</div><div>+ /// isBinOpWithFlags - Returns true if the opcode is a binary operation<br>+ /// with flags.<br>+ static bool isBinOpWithFlags(unsigned Opcode) {<br>
+ switch (Opcode) {<br>+ case ISD::ADD:<br>+ case ISD::MUL:<br>+ case ISD::SUB:<br>+ case ISD::SDIV:<br>+ case ISD::UDIV:<br>+ case ISD::SRL:<br>+ case ISD::SRA:<br>+ case ISD::SHL: return true;<br>
+ default: return false;<br>+ }<br>+ }</div><div> </div><div>B)</div><div>@@ -473,6 +487,19 @@ static void AddNodeIDCustom(FoldingSetNodeID &ID, const SDNode *N) {<br> ID.AddInteger(ST->getPointerInfo().getAddrSpace());<br>
break;<br> }<br>+ case ISD::SDIV:<br>+ case ISD::UDIV:<br>+ case ISD::SRA:<br>+ case ISD::SRL:<br>+ case ISD::MUL:<br>+ case ISD::ADD:<br>+ case ISD::SUB:<br>+ case ISD::SHL: {<br>+ const BinarySDNode *BinNode = static_cast<const BinarySDNode*>(N);<br>
+ AddBinaryNodeIDCustom(ID, N->getOpcode(), BinNode->hasNoUnsignedWrap(),<br>+ BinNode->hasNoSignedWrap(), BinNode->isExact());<br>+ break;<br>+ }<br> case ISD::ATOMIC_CMP_SWAP:<br>
case ISD::ATOMIC_SWAP:<br> case ISD::ATOMIC_LOAD_ADD:</div><div> </div><div>and</div><div> </div><div>C)</div><div>+ switch (Op->getOpcode()) {<br>+ case ISD::ADD:<br>+ case ISD::SUB:<br>+ case ISD::MUL:<br>
+ case ISD::SHL: {<br>+ const BinarySDNode *BinNode =<br>+ static_cast<const BinarySDNode*>(Op.getNode());<br>+ if (BinNode->hasNoSignedWrap())<br>+ break;<br>+ }<br>+ default:<br>
+ NeedOF = true;<br>+ break;<br>+ }</div><div> </div><div>I don't like this code duplication.</div><div>Maybe using #define CASE_BINOP case ISD::SDIV:\<br> case ISD::UDIV: \</div><div>....case ISD::SHL</div>
<div>to avoid this code duplication?</div><div>But I'm not sure this is good idea...</div><div>One more question: why we have difference between A) B) and C)?</div><div> </div><div> </div><div> </div><div> </div></div>
<div><br><br><div>On Tue, Jun 3, 2014 at 4:01 AM, Marcello Maggioni <span dir="ltr"><<a>hayarms@gmail.com</a>></span> wrote:<br>
<blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div dir="ltr"><span style="font-family:arial,sans-serif;font-size:13px">Hi, thanks Daniil for your review!</span><div style="font-family:arial,sans-serif;font-size:13px">
<br></div><div style="font-family:arial,sans-serif;font-size:13px">
1) Yeah, I also kind of like what you proposed more ... I used the approach that is used currently in BinaryOperator for that. I'm not sure if it is better to change both to this format or having them to diverge (keeping BinaryOperator like it was and changing this one to the syntax you proposed).</div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">2) Thanks for spotting those stylistic inconsistencies! I'm working on a new setup and I think I still have to tune some of my editor default settings ...</div>
<div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">3) What you mean exactly with this one?</div><div style="font-family:arial,sans-serif;font-size:13px">
<br></div><div style="font-family:arial,sans-serif;font-size:13px">PS = Here are some updated patches with concerns number 1 and 2 addressed.</div><div style="font-family:arial,sans-serif;font-size:13px"><br></div><div style="font-family:arial,sans-serif;font-size:13px">
Thanks,</div><div style="font-family:arial,sans-serif;font-size:13px">Marcello</div></div><div><br><br><div>2014-06-02 17:00 GMT-07:00 Marcello Maggioni <span dir="ltr"><<a>hayarms@gmail.com</a>></span>:<br>
<blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid"><div dir="ltr">Hi, thanks Daniil for your review!<div><br>
</div><div>1) Yeah, I also kind of like what you proposed more ... I used the approach that is used currently in BinaryOperator for that. I'm not sure if it is better to change both to this format or having them to diverge (keeping BinaryOperator like it was and changing this one to the syntax you proposed).</div>
<div><br></div><div>2) Thanks for spotting those stylistic inconsistencies! I'm working on a new setup and I think I still have to tune some of my editor default settings ...</div><div><br></div><div>3) What you mean exactly with this one?</div>
<div><br></div><div>PS = Here are some updated patches with concerns number 1 and 2 addressed.</div><div><br></div><div>Thanks,</div>
<div>Marcello</div></div><div><br><br><div>2014-06-02 2:36 GMT-07:00 Данил Трошков <span dir="ltr"><<a>troshkovdanil@mail.ru</a>></span>:<div>
<div><br>
<blockquote style="margin:0px 0px 0px 0.8ex;padding-left:1ex;border-left-color:rgb(204,204,204);border-left-width:1px;border-left-style:solid">
<div><div><div>1)<div><br> (SubclassData & ~NUW) | (b * NUW);<br></div>Maybe better to write somehow<div><br>(SubclassData & ~NUW) | (b ? NUW : 0);<br></div>IMHO it is more readable<br><br>2) const BinarySDNode *BinNode = static_cast<const BinarySDNode*>(N);<br>
formatting: \t...<br>+ BinNode->hasNoUnsignedWrap(),<br>+ BinNode->hasNoSignedWrap(),<br>+ BinNode->isExact());<br>the same...<br>+ bool nsw, bool exact) {<br>+ bool nsw, bool exact) {<br>
+ Op1, Op2, HasNUW, HasNSW, IsExact);<br>+ if (BinNode->hasNoSignedWrap())<br>+ break;<br><br>3) Maybe better to remove bundle of cases on low level?<br>In order to reduce duplication of code...<br><br><br><br>
</div></div>
Sat, 31 May 2014 19:30:24 -0700 от Marcello Maggioni <<a>hayarms@gmail.com</a>>:<br>
<blockquote style="margin:10px;padding:0px 0px 0px 10px;border-left-color:rgb(8,87,166);border-left-width:1px;border-left-style:solid">
<div>
<div>
<div><div><div>
<div><div dir="ltr">Here an updated patch.<div><br></div><div>The changes are:</div><div><br></div><div>- I stripped away the new getBinaryNode function and integrated the functionality in the getNode() function with two operands.</div>
<div>- I removed the refactoring for the binary folding logic. Removing getBinaryNode() makes putting that logic in an external function not needed. (this also fixes the clang-format problem Owen pointed out).</div><div>
- Cleaned up some stuff and fixe</div></div></div></div></div></div></div></div></blockquote></div></blockquote></div></div></div></div></blockquote></div></div></blockquote></div></div></blockquote></div></div></div>
</blockquote></div></div></div><br></div>
</blockquote></div><br></div>
</div></div></blockquote></div></div></div><br></div>
</blockquote></div><br></div>