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

[Christian Hagedorn]

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