<br>Ciao Andrea :)<div><br>Il mercoledì 4 giugno 2014, Andrea Di Biagio <<a href="mailto:andrea.dibiagio@gmail.com">andrea.dibiagio@gmail.com</a>> ha scritto:<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>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><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>Will do with the next version of the patch ! Thanks</div><div><br></div><div>Marcello</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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>
>>>>>> + > _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="javascript:;" onclick="_e(event, 'cvml', 'llvm-commits@cs.uiuc.edu')">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>
</blockquote></div>