<HTML><BODY>+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 <hayarms@gmail.com>:<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 class="js-helper js-readmsg-msg">
<style type="text/css"></style>
<div>
<base href="https://e.mail.ru/" target="_self">
<div id="style_14020382270000000390_BODY"><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="//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="//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>
<base href="https://e.mail.ru/" target="_self">
</div>
</div>
</div>
</blockquote>
<br></BODY></HTML>