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

Marcello Maggioni hayarms at gmail.com
Fri Jun 6 08:38:15 PDT 2014


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
> <javascript:;>> wrote:
> > Done
> >
> >
> > 2014-06-06 0:38 GMT-07:00 Данил Трошков <troshkovdanil at mail.ru
> <javascript:;>>:
> >
> >> +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
> <javascript:;>>:
> >>
> >> 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
> <javascript:;>>:
> >>
> >> On Thu, Jun 5, 2014 at 8:50 PM, Marcello Maggioni <hayarms at gmail.com
> <javascript:;>>
> >> 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/5c13a81a/attachment.html>


More information about the llvm-commits mailing list