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

[Marcello Maggioni]

Ciao Andrea :)

Il mercoledì 4 giugno 2014, Andrea Di Biagio <andrea.dibiagio at gmail.com> ha
scritto:

> Ciao Marcello,
>
> I have two minor comments related to your change in
> lib/Target/X86/X86ISelLowering.cpp:
>
> +          static_cast<const BinarySDNode*>(Op.getNode());
>
> I noticed that you use a static_cast there. I guess that is because
> method 'classof' is not implemented for class BinarySDNode.
> Have you considered the possibility of adding that static method to
> BinarySDNode? That would allow you to use llvm style cast instead.
> That said, the static_cast is ok in that particular case and I don't
> have a strong opinion about it.


I wanted to do exactly what you said about this, the problem is that I
didn't find any explicit list of those opcodes that are BinarySDNodes in
the DAG. Excluding the obvious ISD::ADD , SUB .... etc I'm not sure where
to find an exhaustive list of those . Devising them from code is difficult
and missing some opcodes seems easy (everything that calls the 2 operand
variant of getNode generates it, but also the array version with 2
elements, which is more difficult to identify in code ).
I think this is probably the main reason this hasn't being done yet :P
I'll try to search if there is a reliable list.of those somewhere . If you
have clues on this are appreciated.


> About the new LLVM test you added:
>
> You can clean it a bit.
>
> +attributes #0 = { nounwind readnone ssp uwtable
> "less-precise-fpmad"="false" "no-frame-pointer-elim"="true"
> "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false"
> "no-nans-fp-math"="false" "stack-protector-buffer-size"="8"
> "unsafe-fp-math"="false" "use-soft-float"="false" }
> +
> +!llvm.ident = !{!0}
> +
> +!0 = metadata !{metadata !"clang version 3.5.0 "}
>
> You don't need to specify 'llvm.ident'. Also, you can completely get
> rid of the attributes (if you remove #0 the test should still work).
>
> +
> +target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
> +
>
> You can also remove the line with 'target datalayout'.
>
>
Will do with the next version of the patch ! Thanks

Marcello

Cheers,
> Andrea
>
> On Wed, Jun 4, 2014 at 9:28 AM, Marcello Maggioni <hayarms at gmail.com>
> wrote:
> > Whoops, the autocorrection got me :P
> >
> > What I meant is: if somebody with commit access agrees with this patch
> can
> > commit it please? :)
> >
> > Thanks,
> > Marcello
> >
> >
> > 2014-06-04 0:46 GMT-07:00 Marcello Maggioni <hayarms at gmail.com>:
> >
> >> Thank you very much for your comments Daniil !
> >>
> >> If anybody that commit access agrees also with these patches can
> somebody
> >> commit them please? :)
> >>
> >> Cheers,
> >> Marcello
> >>
> >>
> >> 2014-06-04 0:35 GMT-07:00 Daniil Troshkov <troshkovdanil at gmail.com>:
> >>
> >>> ok
> >>>
> >>>
> >>> On Wed, Jun 4, 2014 at 10:30 AM, Marcello Maggioni <hayarms at gmail.com>
> >>> wrote:
> >>>>
> >>>> Here is an updated patch with the cases grouped in a macro.
> >>>>
> >>>> I made the syntax of the macro such that it looks like when put in a
> >>>> switch it looks like  a regular switch case.
> >>>>
> >>>> I couldn't find a better way to address this sadly :/
> >>>>
> >>>> The optimization patch remained the same, but I updated the version
> >>>> number.
> >>>>
> >>>> Marcello
> >>>>
> >>>>
> >>>> 2014-06-03 8:04 GMT-07:00 Marcello Maggioni <hayarms at gmail.com>:
> >>>>
> >>>>> Thanks. The macro could be an idea for that, yeah. I'll try to see if
> >>>>> there is an alternative solution for this and in case I cannot find
> one I
> >>>>> will implement your suggestion.
> >>>>>
> >>>>> About the differences:
> >>>>>
> >>>>> A and B should be the same (probably the order of the labels is
> >>>>> different, but they are the same) and these represent the nodes that
> have
> >>>>> binop flags attached to them.
> >>>>>
> >>>>> In the optimisation code the switch cases are different because I was
> >>>>> targeting a specific case identified by those instructions. (A
> subset of the
> >>>>> nodes that have flags)
> >>>>>
> >>>>> Marcello
> >>>>>
> >>>>> Il martedì 3 giugno 2014, Daniil Troshkov <troshkovdanil at gmail.com>
> ha
> >>>>> scritto:
> >>>>>
> >>>>>> 1) Ok
> >>>>>> 2) Ok
> >>>>>> 3) I mean:
> >>>>>>
> >>>>>> A)
> >>>>>> +  /// isBinOpWithFlags - Returns true if the opcode is a binary
> >>>>>> operation
> >>>>>> +  /// with flags.
> >>>>>> +  static bool isBinOpWithFlags(unsigned Opcode) {
> >>>>>> +    switch (Opcode) {
> >>>>>> +    case ISD::ADD:
> >>>>>> +    case ISD::MUL:
> >>>>>> +    case ISD::SUB:
> >>>>>> +    case ISD::SDIV:
> >>>>>> +    case ISD::UDIV:
> >>>>>> +    case ISD::SRL:
> >>>>>> +    case ISD::SRA:
> >>>>>> +    case ISD::SHL: return true;
> >>>>>> +    default: return false;
> >>>>>> +    }
> >>>>>> +  }
> >>>>>>
> >>>>>> B)
> >>>>>> @@ -473,6 +487,19 @@ static void AddNodeIDCustom(FoldingSetNodeID
> &ID,
> >>>>>> const SDNode *N) {
> >>>>>>      ID.AddInteger(ST->getPointerInfo().getAddrSpace());
> >>>>>>      break;
> >>>>>>    }
> >>>>>> +  case ISD::SDIV:
> >>>>>> + > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu <javascript:;>
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140604/46076400/attachment.html>