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

[Vladimir Kozlov]

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.

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