<div dir="ltr">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.<div><br></div><div>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()</div>
<div><br></div><div>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. </div><div><br></div><div>
It will also have a classof() checking for the opcodes of the binary operations with flags (that we know which are).</div><div><br></div><div>In the patches attached I implemented this paradigm and also fixed the test Andrea mentioned.</div>
<div><br></div><div>Let me know what you think of this approach :)</div><div><br></div><div>Marcello</div><div><br></div></div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-06-04 9:20 GMT-07:00 Marcello Maggioni <span dir="ltr"><<a href="mailto:hayarms@gmail.com" target="_blank">hayarms@gmail.com</a>></span>:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>Ciao Andrea :)<div><br>Il mercoledì 4 giugno 2014, Andrea Di Biagio <<a href="mailto:andrea.dibiagio@gmail.com" target="_blank">andrea.dibiagio@gmail.com</a>> ha scritto:<div class="">
<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Ciao Marcello,<br>
<br>
I have two minor comments related to your change in<br>
lib/Target/X86/X86ISelLowering.cpp:<br>
<br>
+ static_cast<const BinarySDNode*>(Op.getNode());<br>
<br>
I noticed that you use a static_cast there. I guess that is because<br>
method 'classof' is not implemented for class BinarySDNode.<br>
Have you considered the possibility of adding that static method to<br>
BinarySDNode? That would allow you to use llvm style cast instead.<br>
That said, the static_cast is ok in that particular case and I don't<br>
have a strong opinion about it.</blockquote><div><br></div></div><div>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 ). </div>
<div>I think this is probably the main reason this hasn't being done yet :P</div><div>I'll try to search if there is a reliable list.of those somewhere . If you have clues on this are appreciated.</div><div class="">
<div><br></div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
About the new LLVM test you added:<br>
<br>
You can clean it a bit.<br>
<br>
+attributes #0 = { nounwind readnone ssp uwtable<br>
"less-precise-fpmad"="false" "no-frame-pointer-elim"="true"<br>
"no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false"<br>
"no-nans-fp-math"="false" "stack-protector-buffer-size"="8"<br>
"unsafe-fp-math"="false" "use-soft-float"="false" }<br>
+<br>
+!llvm.ident = !{!0}<br>
+<br>
+!0 = metadata !{metadata !"clang version 3.5.0 "}<br>
<br>
You don't need to specify 'llvm.ident'. Also, you can completely get<br>
rid of the attributes (if you remove #0 the test should still work).<br>
<br>
+<br>
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"<br>
+<br>
<br>
You can also remove the line with 'target datalayout'.<br>
<br></blockquote><div><br></div></div><div>Will do with the next version of the patch ! Thanks</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>Marcello</div><div><br></div></font></span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5">
Cheers,<br>
Andrea<br>
<br>
On Wed, Jun 4, 2014 at 9:28 AM, Marcello Maggioni <<a>hayarms@gmail.com</a>> wrote:<br>
> Whoops, the autocorrection got me :P<br>
><br>
> What I meant is: if somebody with commit access agrees with this patch can<br>
> commit it please? :)<br>
><br>
> Thanks,<br>
> Marcello<br>
><br>
><br>
> 2014-06-04 0:46 GMT-07:00 Marcello Maggioni <<a>hayarms@gmail.com</a>>:<br>
><br>
>> Thank you very much for your comments Daniil !<br>
>><br>
>> If anybody that commit access agrees also with these patches can somebody<br>
>> commit them please? :)<br>
>><br>
>> Cheers,<br>
>> Marcello<br>
>><br>
>><br>
>> 2014-06-04 0:35 GMT-07:00 Daniil Troshkov <<a>troshkovdanil@gmail.com</a>>:<br>
>><br>
>>> ok<br>
>>><br>
>>><br>
>>> On Wed, Jun 4, 2014 at 10:30 AM, Marcello Maggioni <<a>hayarms@gmail.com</a>><br>
>>> wrote:<br>
>>>><br>
>>>> Here is an updated patch with the cases grouped in a macro.<br>
>>>><br>
>>>> I made the syntax of the macro such that it looks like when put in a<br>
>>>> switch it looks like a regular switch case.<br>
>>>><br>
>>>> I couldn't find a better way to address this sadly :/<br>
>>>><br>
>>>> The optimization patch remained the same, but I updated the version<br>
>>>> number.<br>
>>>><br>
>>>> Marcello<br>
>>>><br>
>>>><br>
>>>> 2014-06-03 8:04 GMT-07:00 Marcello Maggioni <<a>hayarms@gmail.com</a>>:<br>
>>>><br>
>>>>> Thanks. The macro could be an idea for that, yeah. I'll try to see if<br>
>>>>> there is an alternative solution for this and in case I cannot find one I<br>
>>>>> will implement your suggestion.<br>
>>>>><br>
>>>>> About the differences:<br>
>>>>><br>
>>>>> A and B should be the same (probably the order of the labels is<br>
>>>>> different, but they are the same) and these represent the nodes that have<br>
>>>>> binop flags attached to them.<br>
>>>>><br>
>>>>> In the optimisation code the switch cases are different because I was<br>
>>>>> targeting a specific case identified by those instructions. (A subset of the<br>
>>>>> nodes that have flags)<br>
>>>>><br>
>>>>> Marcello<br>
>>>>><br>
>>>>> Il martedì 3 giugno 2014, Daniil Troshkov <<a>troshkovdanil@gmail.com</a>> ha<br>
>>>>> scritto:<br>
>>>>><br>
>>>>>> 1) Ok<br>
>>>>>> 2) Ok<br>
>>>>>> 3) I mean:<br>
>>>>>><br>
>>>>>> A)<br>
>>>>>> + /// isBinOpWithFlags - Returns true if the opcode is a binary<br>
>>>>>> operation<br>
>>>>>> + /// with flags.<br>
>>>>>> + static bool isBinOpWithFlags(unsigned Opcode) {<br>
>>>>>> + switch (Opcode) {<br>
>>>>>> + case ISD::ADD:<br>
>>>>>> + case ISD::MUL:<br>
>>>>>> + case ISD::SUB:<br>
>>>>>> + case ISD::SDIV:<br>
>>>>>> + case ISD::UDIV:<br>
>>>>>> + case ISD::SRL:<br>
>>>>>> + case ISD::SRA:<br>
>>>>>> + case ISD::SHL: return true;<br>
>>>>>> + default: return false;<br>
>>>>>> + }<br>
>>>>>> + }<br>
>>>>>><br>
>>>>>> B)<br>
>>>>>> @@ -473,6 +487,19 @@ static void AddNodeIDCustom(FoldingSetNodeID &ID,<br>
>>>>>> const SDNode *N) {<br>
>>>>>> ID.AddInteger(ST->getPointerInfo().getAddrSpace());<br>
>>>>>> break;<br>
>>>>>> }<br>
>>>>>> + case ISD::SDIV:<br></div></div>
>>>>>> + > _______________________________________________<div class=""><br>
> llvm-commits mailing list<br>
> <a>llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
><br>
</div></blockquote></div>
</blockquote></div><br></div>