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

Marcello Maggioni hayarms at gmail.com
Thu Jun 5 04:50:27 PDT 2014


After analyzing the situation a bit I think that maybe is not the best
solution to extend the functionality inside of the BinarySDNode class
directly.

The BinarySDNode class specifies clearly in its comment that it is meant
only to exist to allocate the two SDUses of the operands together, which is
probably why it doesn't have a classof()

Another idea (implemented in the attached round of patch), could be to
implement a new class called BinaryWithFlagsSDNode that extends from
 BinarySDNode and that has the flags.

It will also have a classof() checking for the opcodes of the binary
operations with flags (that we know which are).

In the patches attached I implemented this paradigm and also fixed the test
Andrea mentioned.

Let me know what you think of this approach :)

Marcello



2014-06-04 9:20 GMT-07:00 Marcello Maggioni <hayarms at gmail.com>:

>
> 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
>> > 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/20140605/92d54e94/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: flag_nodes_v4.patch
Type: application/octet-stream
Size: 14647 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140605/92d54e94/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: x86_opt_wrap_v4.patch
Type: application/octet-stream
Size: 2355 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140605/92d54e94/attachment-0001.obj>


More information about the llvm-commits mailing list