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

[Vladimir Kozlov]

c1_Compilation.hpp: looks like both versions of allocator() do the same thing.

I suggest to build with configure --with-debug-level=optimized to check that NOT_PRODUCT can be built with these changes.

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