RFR(S) 8032656: Tag the MachSpillCopies with purpose information

[Niclas Adlertz]

Ok, thank you.

Kind Regards,
Niclas Adlertz

On 2014-01-29 23:34, Vladimir Kozlov wrote:
> Looks fine. NOTE: our repo is closed. Please, hold with the push.
>
> Thanks,
> Vlaidmir
>
> On 1/29/14 2:27 PM, Niclas Adlertz wrote:
>> Hi Vladimir,
>>
>> Thank you.
>> Here is the new
>> webrev:http://cr.openjdk.java.net/~adlertz/JDK-8032656/webrev02/
>> I also included machnode.hpp in chaitin.hpp since we need the definition
>> of MachSpillCopyNode::SpillType. (Build failed on Solaris otherwise).
>>
>> Kind Regards,
>> Niclas Adlertz
>>
>> On 2014-01-29 19:46, Vladimir Kozlov wrote:
>>> Niclas,
>>>
>>> Changes are fine but names should be adjusted. I think we should use
>>> original "SpillCopy" suffix instead of "SpillNode". "RegToMem" and
>>> "MemToReg" are missing the suffux.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 1/29/14 3:14 AM, Niclas Adlertz wrote:
>>>> Hi Vladimir K,
>>>>
>>>> Thank you for your response.
>>>>
>>>> Here is an updated webrev:
>>>> http://cr.openjdk.java.net/~adlertz/JDK-8032656/webrev01/
>>>>
>>>> Kind Regards,
>>>> Niclas Adlertz
>>>>
>>>>
>>>> On 2014-01-28 18:21, Vladimir Kozlov wrote:
>>>>> On 1/28/14 3:18 AM, Niclas Adlertz wrote:
>>>>>> Hi Vladimir,
>>>>>>
>>>>>> Thank you for your response.
>>>>>> That is a possibility as well.
>>>>>>
>>>>>> By using an enum I still have the problem of deciding what to return
>>>>>> in Name().
>>>>>> I could have either
>>>>>> * an array of const char* with the names and index into it by using
>>>>>> the enum when returning a name
>>>>>> * a switch-case on the enums in the Name() method.
>>>>>
>>>>> Switch by enum in Name().
>>>>>
>>>>>>
>>>>>> However, I think sub-classing the MachSpillCopyNode looks cleaner.
>>>>>> The sub-classing approach could also be of use in a product build
>>>>>> when debugging a crash and we want to check what type
>>>>>> of node we are at.
>>>>>
>>>>> You can determine type by enum value too.
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>>>
>>>>>> If it's ok with you, I'll wait and see what the others think. If more
>>>>>> people think the enum approach is better I can use
>>>>>> enums instead.
>>>>>>
>>>>>> Kind Regards,
>>>>>> Niclas Adlertz
>>>>>>
>>>>>> On 2014-01-28 11:50, Vladimir Ivanov wrote:
>>>>>>> Niclas,
>>>>>>>
>>>>>>> Why didn't you introduce an enum instead and pass the reason to
>>>>>>> constructor? Introducing 13 subclasses just to
>>>>>>> overload Name() method in debug builds looks like an overkill to me.
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Vladimir Ivanov
>>>>>>>
>>>>>>> On 1/28/14 2:07 PM, Niclas Adlertz wrote:
>>>>>>>> Hi all,
>>>>>>>>
>>>>>>>> When debugging or visualizing spills/splits/moves in C2 it's hard to
>>>>>>>> know what caused an insertion of a certain MachSpillCopy. I've made
>>>>>>>> this
>>>>>>>> easier by creating subclasses of the MachSpillCopyNode.
>>>>>>>>
>>>>>>>> webrev: http://cr.openjdk.java.net/~adlertz/JDK-8032656/webrev00/
>>>>>>>> bug: https://bugs.openjdk.java.net/browse/JDK-8032656
>>>>>>>>
>>>>>>>> Kind Regards,
>>>>>>>> Niclas Adlertz
>>>>>>
>>>>
>>