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

[Andrea Di Biagio]

Thanks Marcello,
the patch looks good to me.

On Fri, Jun 6, 2014 at 10:01 AM, Marcello Maggioni <hayarms at gmail.com> wrote:
> Done
>
>
> 2014-06-06 0:38 GMT-07:00 Данил Трошков <troshkovdanil at mail.ru>:
>
>> +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.
>>
>>
>>
>