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

[Christian Hagedorn]

Thank you Vladimir for your careful review!

Best regards,
Christian

On 19.08.20 18:43, Vladimir Kozlov wrote:
> 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
>>>>>>>>