[16] RFR(S): 8251093: Improve C1 register allocator logging and debugging support
Christian Hagedorn
christian.hagedorn at oracle.com
Thu Aug 20 07:10:57 UTC 2020
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
>>>>>>>>
More information about the hotspot-compiler-dev
mailing list