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

[Christian Hagedorn]

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