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

[Vladimir Kozlov]

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
>>>>
>>>
>