Re[2]: [PATCH] Passing down BinaryOperator flags to BinarySDNode + X86 optimization

[Данил Трошков]

 +while.body:                                       ; preds = %while.body.preheader, %while.body
where is while.body.preheader ? May be it is a good idea remove comments at all...


Fri, 6 Jun 2014 00:03:44 -0700 от Marcello Maggioni <hayarms at gmail.com>:
>Hello, 
>
>here I have updated the patch with the suggestions you pointed out addressed.
>
>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.
>
>Let me know what you think!
>
>Cheers,
>Marcello
>
>
>2014-06-05 13:49 GMT-07:00 Andrea Di Biagio  < andrea.dibiagio at gmail.com > :
>>On Thu, Jun 5, 2014 at 8:50 PM, Marcello Maggioni < hayarms at gmail.com > wrote:
>>> Hi Andrea,
>>>
>>> answers are inlined!
>>>
>>> <snip>
>>>> About patch "flag_nodes_v4.patch"
>>>>
>>>> In include/llvm/CodeGen/SelectionDAG.h:
>>>>
>>>> +  /// isBinOpWithFlags - Returns true if the opcode is a binary operation
>>>> +  /// with flags.
>>>> +  static bool isBinOpWithFlags(unsigned Opcode) {
>>>> +    switch (Opcode) {
>>>> +    case BINOP_NODES_W_FLAGS: return true;
>>>> +    default: return false;
>>>> +    }
>>>> +  }
>>>> +
>>>>
>>>> I think you can move that definition in SelectionDAGNodes.h. You can
>>>> simplify the code in method 'BinaryWithFlagsSDNode::classof' If you
>>>> can make that definition visible. For example, method '
>>>> BinaryWithFlagsSDNode ::classof'  could be rewritten as follows:
>>>>
>>>> //
>>>>   static bool classof(const SDNode *N) {
>>>>     return isBinOpWithFlags(N->getOpcode());
>>>> //
>>>>
>>>> Also, if possible, please remove macro BINOP_NODES_W_FLAGS. In my
>>>> opinion it doesn't make the code more readable.
>>>
>>>
>>> About that ... BINOP_NODES_W_FLAGS is used in a couple of places (in
>>> switches) where mainly I cannot avoid to have the case labels there (I would
>>> like to use isBinOpWithFlags() everywhere, but in switches is not possible
>>> and putting the check outside the switch would be less performant, so I
>>> prefer having the cases there.
>>>
>>> Do you think just duplicating the cases in those couple of places would be
>>> ok?
>>
>>Yes, that was the idea. I don't think it is bad to have those cases
>>duplicated in 'AddNodeIDCustom' and 'isBinOpWithFlags' . We already do
>>something similar for other dag nodes (see for example AtomicSDNode).
>>That said, your approach is not wrong. It is just that, in general, I
>>prefer not to use macros when possible.
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140606/45c169c1/attachment.html>