<div dir="ltr">Done</div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-06 0:38 GMT-07:00 Данил Трошков <span dir="ltr"><<a href="mailto:troshkovdanil@mail.ru" target="_blank">troshkovdanil@mail.ru</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>+while.body: ; preds = %while.body.preheader, %while.body<br>where is while.body.preheader ? May be it is a good idea remove comments at all...<br><br><br>Fri, 6 Jun 2014 00:03:44 -0700 от Marcello Maggioni <<a href="mailto:hayarms@gmail.com" target="_blank">hayarms@gmail.com</a>>:<div>
<div class="h5"><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 dir="ltr">Hello, <div><br></div><div>here I have updated the patch with the suggestions you pointed out addressed.</div><div><br></div><div>The only think I skipped is using \brief in the comment of isBinOpWithFlags,because every other function in the file uses the style it is actually uses, so probably I think that keeping everything consistent is quite a good idea.</div>
<div><br></div><div>Let me know what you think!</div><div><br></div><div>Cheers,</div><div>Marcello</div></div><div><br><br><div>2014-06-05 13:49 GMT-07:00 Andrea Di Biagio <span dir="ltr"><<a href="https://e.mail.ru/compose/?mailto=mailto%3aandrea.dibiagio@gmail.com" target="_blank">andrea.dibiagio@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>On Thu, Jun 5, 2014 at 8:50 PM, Marcello Maggioni <<a href="https://e.mail.ru/compose/?mailto=mailto%3ahayarms@gmail.com" target="_blank">hayarms@gmail.com</a>> wrote:<br>
> Hi Andrea,<br>
><br>
> answers are inlined!<br>
><br>
</div>> <snip><br>
<div><div>>> About patch "flag_nodes_v4.patch"<br>
>><br>
>> In include/llvm/CodeGen/SelectionDAG.h:<br>
>><br>
>> + /// isBinOpWithFlags - Returns true if the opcode is a binary operation<br>
>> + /// with flags.<br>
>> + static bool isBinOpWithFlags(unsigned Opcode) {<br>
>> + switch (Opcode) {<br>
>> + case BINOP_NODES_W_FLAGS: return true;<br>
>> + default: return false;<br>
>> + }<br>
>> + }<br>
>> +<br>
>><br>
>> I think you can move that definition in SelectionDAGNodes.h. You can<br>
>> simplify the code in method 'BinaryWithFlagsSDNode::classof' If you<br>
>> can make that definition visible. For example, method '<br>
>> BinaryWithFlagsSDNode ::classof' could be rewritten as follows:<br>
>><br>
>> //<br>
>> static bool classof(const SDNode *N) {<br>
>> return isBinOpWithFlags(N->getOpcode());<br>
>> //<br>
>><br>
>> Also, if possible, please remove macro BINOP_NODES_W_FLAGS. In my<br>
>> opinion it doesn't make the code more readable.<br>
><br>
><br>
> About that ... BINOP_NODES_W_FLAGS is used in a couple of places (in<br>
> switches) where mainly I cannot avoid to have the case labels there (I would<br>
> like to use isBinOpWithFlags() everywhere, but in switches is not possible<br>
> and putting the check outside the switch would be less performant, so I<br>
> prefer having the cases there.<br>
><br>
> Do you think just duplicating the cases in those couple of places would be<br>
> ok?<br>
<br>
</div></div>Yes, that was the idea. I don't think it is bad to have those cases<br>
duplicated in 'AddNodeIDCustom' and 'isBinOpWithFlags' . We already do<br>
something similar for other dag nodes (see for example AtomicSDNode).<br>
That said, your approach is not wrong. It is just that, in general, I<br>
prefer not to use macros when possible.<br>
</blockquote></div><br></div>
</div>
</div>
</div>
</div>
</blockquote>
<br></div></div></div>
</blockquote></div><br></div>