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

[Niclas Adlertz]

Thank you Roland.

I've corrected the things you asked me to do.

Kind Regards,
Niclas Adlertz

On 2014-01-23 11:01, Roland Westrelin wrote:
> 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