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

[Niclas Adlertz]

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