Thanks!<div><br></div><div>I don't nave commit access, so if there are no other comments can somebody commit it for me?</div><div><br></div><div>Cheers,</div><div>Marcello<br><br>Il venerdì 6 giugno 2014, Andrea Di Biagio <<a href="mailto:andrea.dibiagio@gmail.com">andrea.dibiagio@gmail.com</a>> ha scritto:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Thanks Marcello,<br>
the patch looks good to me.<br>
<br>
On Fri, Jun 6, 2014 at 10:01 AM, Marcello Maggioni <<a href="javascript:;" onclick="_e(event, 'cvml', 'hayarms@gmail.com')">hayarms@gmail.com</a>> wrote:<br>
> Done<br>
><br>
><br>
> 2014-06-06 0:38 GMT-07:00 Данил Трошков <<a href="javascript:;" onclick="_e(event, 'cvml', 'troshkovdanil@mail.ru')">troshkovdanil@mail.ru</a>>:<br>
><br>
>> +while.body:                                       ; preds =<br>
>> %while.body.preheader, %while.body<br>
>> where is while.body.preheader ? May be it is a good idea remove comments<br>
>> at all...<br>
>><br>
>><br>
>> Fri, 6 Jun 2014 00:03:44 -0700 от Marcello Maggioni <<a href="javascript:;" onclick="_e(event, 'cvml', 'hayarms@gmail.com')">hayarms@gmail.com</a>>:<br>
>><br>
>> Hello,<br>
>><br>
>> here I have updated the patch with the suggestions you pointed out<br>
>> addressed.<br>
>><br>
>> The only think I skipped is using \brief in the comment of<br>
>> isBinOpWithFlags,because every other function in the file uses the style it<br>
>> is actually uses, so probably I think that keeping everything consistent is<br>
>> quite a good idea.<br>
>><br>
>> Let me know what you think!<br>
>><br>
>> Cheers,<br>
>> Marcello<br>
>><br>
>><br>
>> 2014-06-05 13:49 GMT-07:00 Andrea Di Biagio <<a href="javascript:;" onclick="_e(event, 'cvml', 'andrea.dibiagio@gmail.com')">andrea.dibiagio@gmail.com</a>>:<br>
>><br>
>> On Thu, Jun 5, 2014 at 8:50 PM, Marcello Maggioni <<a href="javascript:;" onclick="_e(event, 'cvml', 'hayarms@gmail.com')">hayarms@gmail.com</a>><br>
>> wrote:<br>
>> > Hi Andrea,<br>
>> ><br>
>> > answers are inlined!<br>
>> ><br>
>> > <snip><br>
>> >> 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<br>
>> >> 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<br>
>> > would<br>
>> > like to use isBinOpWithFlags() everywhere, but in switches is not<br>
>> > 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<br>
>> > be<br>
>> > ok?<br>
>><br>
>> 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>
>><br>
>><br>
>><br>
><br>
</blockquote></div>