RFR(L) 8031498: Cleanup and re-factorize PhaseChaitin::build_ifg_physical()

[Niclas Adlertz]

Thank you Vladimir.

Kind Regards,
Niclas Adlertz

On 2014-01-22 22:24, Vladimir Kozlov wrote:
> The bug id in the mail's Subject is incorrect. Should be 8031498. I 
> fixed it.
>
> Changes looks good.
>
> Thanks,
> Vladimir
>
> On 1/20/14 4:47 AM, Niclas Adlertz wrote:
>> Thank you Vladimir.
>>
>> I removed the references and removed some of the line breaks.
>>
>> http://cr.openjdk.java.net/~adlertz/JDK-8031498/webrev04/
>>
>> Kind Regards,
>> Niclas Adlertz
>>
>> On 2014-01-18 00:29, Vladimir Kozlov wrote:
>>> On 1/17/14 1:53 AM, Niclas Adlertz wrote:
>>>> Vladimir,
>>>>
>>>> Thank you for your comments!
>>>>
>>>>> Vectors are separate from floats. Please, rename the method to
>>>>> is_float_or_vector().
>>>> Done
>>>>
>>>>>
>>>>> class Pressure. Field names start with '_' to separate then from
>>>>> variables.
>>>>>
>>>> Done
>>>>
>>>>> Why you don't like to path pointer to liveout? It helped only in few
>>>>> places but code in interfere_with_live() become complicated 
>>>>> (IndexSet&
>>>>> liveout and then elements(&liveout)).
>>>> I thought it looked better to send in the liveout as a reference since
>>>> it was allocated on stack in build_ifg_physical:
>>>> IndexSet liveout(_live->live(block));
>>>> But if you want me revert the change I can do that.
>>>
>>> The final consumer IndexSetIterator of liveout needs pointer. You have
>>> to add additional conversion in sources which I don't like:
>>>
>>> foo(IndexSet& liveout) {
>>>   IndexSetIterator elements(&liveout);
>>>
>>> It makes code more complex to understand.
>>>
>>> Also in build_ifg_physical() you have too many empty lines.
>>>
>>> Thanks,
>>> Vladimir
>>>
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~adlertz/JDK-8031498/webrev03/
>>>>
>>>> Kind Regards,
>>>> Niclas Adlertz
>>>>
>>>>
>>>> On 2014-01-16 09:11, Vladimir Kozlov wrote:
>>>>> Niclas,
>>>>>
>>>>> Changes are good. I have few notes.
>>>>>
>>>>> Vectors are separate from floats. Please, rename the method to
>>>>> is_float_or_vector().
>>>>>
>>>>> class Pressure. Field names start with '_' to separate then from
>>>>> variables.
>>>>>
>>>>> Why you don't like to path pointer to liveout? It helped only in few
>>>>> places but code in interfere_with_live() become complicated 
>>>>> (IndexSet&
>>>>> liveout and then elements(&liveout)).
>>>>>
>>>>> Thanks,
>>>>> Vladimir
>>>>>
>>>>> On 1/15/14 5:45 AM, Niclas Adlertz wrote:
>>>>>> Hi again,
>>>>>>
>>>>>> I found a commented section that I forgot to remove in chaitin.hpp:
>>>>>>
>>>>>> // stores the first low-to-high transition (starting from the top of
>>>>>> block)
>>>>>> //uint final_high_pressure_index;
>>>>>>
>>>>>> Updated the webrev:
>>>>>> http://cr.openjdk.java.net/~adlertz/JDK-8031498/webrev02/
>>>>>>
>>>>>> On 2014-01-15 14:26, Niclas Adlertz wrote:
>>>>>>> Hi Rickard,
>>>>>>>
>>>>>>> Thank you for your comments.
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~adlertz/JDK-8031498/webrev01/
>>>>>>>
>>>>>>> Kind Regards,
>>>>>>> Niclas Adlertz
>>>>>>>
>>>>>>> On 2014-01-14 15:03, Rickard Bäckman wrote:
>>>>>>>> Just a few comments on the C++ parts. I haven't verified that the
>>>>>>>> logic
>>>>>>>> is still the same.
>>>>>>>>
>>>>>>>> 1) It seems like there are a couple of functions that could be
>>>>>>>> created
>>>>>>>> in the new Pressure class to avoid code duplication. One 
>>>>>>>> example is
>>>>>>>> the
>>>>>>>> check_for_high_pressure_block method.
>>>>>>>>
>>>>>>>> 2) All places that checks lrg._is_float also check lrg._is_vector.
>>>>>>>> That
>>>>>>>> check could be made into a boolean method on the LRG instead. The
>>>>>>>> same
>>>>>>>> for is_UP() & mask_size().
>>>>>>>>
>>>>>>>> 3) To make code more generic the int_pressure and float_pressure
>>>>>>>> instances could be passed the INTPRESSURE and FLOATPRESSURE and 
>>>>>>>> keep
>>>>>>>> those as instance variables.
>>>>>>>>
>>>>>>>> Good work.
>>>>>>>>
>>>>>>>> /R
>>>>>>>>
>>>>>>>> On 01/14, Niclas Adlertz wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> This is a first step to clean up the register allocator in C2. In
>>>>>>>>> this change we have divided the very long method
>>>>>>>>> build_ifg_physical() into many smaller ones, making it easier to
>>>>>>>>> see the steps when building the IFG (and computing
>>>>>>>>> the block pressure).
>>>>>>>>> We have also cleaned up old comments and improved old code, both
>>>>>>>>> by better naming and by simplifying expressions.
>>>>>>>>> I personally think that the easiest way to review this change is
>>>>>>>>> to have the old and new ifg.cpp side by side, and
>>>>>>>>> start from build_ifg_physical().
>>>>>>>>> The next step is to create a new class for these methods,
>>>>>>>>> isolating them and removing them from PhaseChaitin.
>>>>>>>>> WEBREV: http://cr.openjdk.java.net/~adlertz/JDK-8031498/webrev00/
>>>>>>>>> BUG: https://bugs.openjdk.java.net/browse/JDK-8031498
>>>>>>>>>
>>>>>>>>> Kind Regards,
>>>>>>>>> Niclas Adlertz
>>>>>>>
>>>>>>
>>>>
>>