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

[Vladimir Kozlov]

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