[16] RFR(S): 8251093: Improve C1 register allocator logging and debugging support

[Vladimir Kozlov]

Looks good.

Thanks,
Vladimir K

On 8/19/20 7:06 AM, Christian Hagedorn wrote:
> On 18.08.20 17:41, Vladimir Kozlov wrote:
>> c1_Compilation.hpp: looks like both versions of allocator() do the same thing.
> 
> Right, I first wanted to have a public allocator() version in non-product only - but that might be over-engineered as 
> they do the same thing. I changed it back to a single public version.
> 
>> I suggest to build with configure --with-debug-level=optimized to check that NOT_PRODUCT can be built with these changes.
> 
> That's a good idea! I indeed forgot about one NOT_PRODUCT -> DEBUG_ONLY change. I also found other build issues with the 
> optimized build. I filed [1] and already sent an RFR for it. It builds successfully with this patch on top of it.
> 
> http://cr.openjdk.java.net/~chagedorn/8251093/webrev.02/
> 
> Best regards,
> Christian
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8252037
> 
>> Thanks,
>> Vladimir
>>
>> On 8/18/20 6:16 AM, Christian Hagedorn wrote:
>>> Hi Vladimir
>>>
>>> On 17.08.20 19:36, Vladimir Kozlov wrote:
>>>> On 8/17/20 12:44 AM, Christian Hagedorn wrote:
>>>>> Hi Vladimir
>>>>>
>>>>> Yes, you're right, these should be changed into ASSERT and DEBUG().
>>>>>
>>>>> I'm wondering though if these ifdefs are even required for if-blocks inside methods?
>>>>>
>>>>> Isn't, for example, this if-block:
>>>>>
>>>>> #ifndef PRODUCT
>>>>>          if (TraceLinearScanLevel >= 2) {
>>>>>            tty->print_cr("killing XMMs for trig");
>>>>>          }
>>>>> #endif
>>>>>
>>>>> removed anyways when the flag is set to < 2 (which is statically known and thus would allow this entire block to be 
>>>>> removed)? Or does it make a difference by explicitly guarding it with an ifdef?
>>>>
>>>> You are right. It could be statically removed. But we keep #ifdef sometimes to indicate that code is executed only 
>>>> in debug build because we don't always remember type of a flag.
>>>
>>> I see, that makes sense. I updated my patch and left the ifdefs there but changed them to ASSERT. I also updated 
>>> other ifdefs belonging to TraceLinearScanLevel appropriately.
>>>
>>> http://cr.openjdk.java.net/~chagedorn/8251093/webrev.01/
>>>
>>> Best regards,
>>> Christian
>>>
>>>>
>>>> Thanks,
>>>> Vladimir K
>>>>
>>>>>
>>>>> Best regards,
>>>>> Christian
>>>>>
>>>>> On 14.08.20 20:09, Vladimir Kozlov wrote:
>>>>>> One note. Most of the code is guarded by #ifndef PRODUCT.
>>>>>>
>>>>>> But the flag is available only in DEBUG build:
>>>>>>    develop(intx, TraceLinearScanLevel, 0,
>>>>>>
>>>>>> Should we use #ifdef ASSERT and DEBUG() instead?
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> On 8/14/20 5:10 AM, Christian Hagedorn wrote:
>>>>>>> Hi
>>>>>>>
>>>>>>> Please review the following enhancement for C1:
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8251093
>>>>>>> http://cr.openjdk.java.net/~chagedorn/8251093/webrev.00/
>>>>>>>
>>>>>>> While I was working on JDK-8249603 [1], I added some additional debugging and logging code which helped to figure 
>>>>>>> out what was going on. I think it would be useful to have this code around for the analysis of future C1 register 
>>>>>>> allocator bugs.
>>>>>>>
>>>>>>> This RFE adds (everything non-product code):
>>>>>>> - find_interval(number): Can be called like that from gdb anywhere to find an interval with the given number.
>>>>>>> - Interval::print_children()/print_parent(): Useful when debugging with gdb to quickly show the split children 
>>>>>>> and parent.
>>>>>>> - LinearScan::print_reg_num(number): Prints the register or stack location for this register number. This is 
>>>>>>> useful in some places (logging with TraceLinearScanLevel set) where it just printed a number which first had to 
>>>>>>> be manually looked up in other logs.
>>>>>>>
>>>>>>> I additionally did some cleanup of the touched code.
>>>>>>>
>>>>>>> We could additionally split the TraceLinearScanLevel flag into separate flags related to the different phases of 
>>>>>>> the register allocation algorithm. It currently just prints too much details on the higher levels. You often find 
>>>>>>> yourself being interested in a specific part of the algorithm and only want to know more details there. To 
>>>>>>> achieve that you now you have to either handle all the noise or manually disable/enable other logs. We could file 
>>>>>>> an RFE to clean this up if it's worth the effort - given that there are not many new issues filed for C1 register 
>>>>>>> allocation today.
>>>>>>>
>>>>>>> Thank you!
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Christian
>>>>>>>
>>>>>>>
>>>>>>> [1] https://bugs.openjdk.java.net/browse/JDK-8251093
>>>>>>>