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

[Roland Westrelin]

Hi Niclas,

It looks good to me. Nice cleanup. A few minor comments. No need to send another webrev:

Isn’t assignment in tests:

294   while ((interfering_lid = elements.next()) != 0) {
399   while ((lidx = elements.next()) != 0) {
414   while ((lidx = elements.next()) != 0) {
507   while ((lid = elements.next()) != 0) {

something we try to avoid?

Messages in asserts are nice:
 439   assert(int_pressure._current_pressure == count_int_pressure(liveout), "" );
 440   assert(float_pressure._current_pressure == count_float_pressure(liveout), "");

if (idx != 0) {

is better than

 627           if (idx) {


Roland.

On Jan 22, 2014, at 10:24 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> 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
>>>>>>> 
>>>>>> 
>>>> 
>>