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

[Andrea Di Biagio]

Hi Marcello,

I submitted your patches at revision 210467 and 210468.

Cheers,
Andrea

On Fri, Jun 6, 2014 at 4:38 PM, Marcello Maggioni <hayarms at gmail.com> wrote:
> Thanks!
>
> I don't nave commit access, so if there are no other comments can somebody
> commit it for me?
>
> Cheers,
> Marcello
>
> Il venerdì 6 giugno 2014, Andrea Di Biagio <andrea.dibiagio at gmail.com> ha
> scritto:
>
>> 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.
>> >>
>> >>
>> >>
>> >