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

Marcello Maggioni hayarms at gmail.com
Fri Jun 6 02:01:36 PDT 2014


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
> <https://e.mail.ru/compose/?mailto=mailto%3aandrea.dibiagio@gmail.com>>:
>
> On Thu, Jun 5, 2014 at 8:50 PM, Marcello Maggioni <hayarms at gmail.com
> <https://e.mail.ru/compose/?mailto=mailto%3ahayarms@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/cfec5136/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: flag_nodes_v6.patch
Type: application/octet-stream
Size: 14789 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140606/cfec5136/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: x86_opt_wrap_v6.patch
Type: application/octet-stream
Size: 2069 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140606/cfec5136/attachment-0001.obj>


More information about the llvm-commits mailing list