RFR: JDK-8217909: Make unused r12 register (without compressed oops) available to regalloc in C2

Wed Feb 20 20:20:55 UTC 2019

>>> Do you need anything special for 32-bit x86 port in x86_32.ad?
>>
>> Not that I know of. r12 and the compressed oops stuff is only ever
>> present in 64bit. We might want to check how it's done on other 64bit
>> platforms though..
> 
> Since you add reg_mask_init() usage into c2_init_x86.cpp, won't its
> absence cause problems at linkage there?

Right! Duh. I'm filing a follow-up bug since I already pushed it.

Roman

> Best regards,
> Vladimir Ivanov
> 
>>>>> On 2/18/19, 12:23 PM, Roman Kennke wrote:
>>>>>> Somehow I missed it when I looked at it back when you posted it. Have
>>>>>> you updated it in place in the meantime?
>>>>> Not that I recall :-)
>>>>>> Anyhow, this looks promising, I will try to take it from there.
>>>>>> Thanks!!
>>>>>>
>>>>>
>>>>> Great, thanks!
>>>>>
>>>>> dl
>>>>>
>>>>>> Roman
>>>>>>
>>>>>>
>>>>>>> You don't see it in the webrev I posted?
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~dlong//8217909/webrev/
>>>>>>>
>>>>>>> dl
>>>>>>>
>>>>>>> On 2/18/19 11:14 AM, Roman Kennke wrote:
>>>>>>>> No I haven't. Have you posted this somewhere and I missed it? :-)
>>>>>>>>
>>>>>>>> I will try it out. Thanks!
>>>>>>>>
>>>>>>>> Roman
>>>>>>>>
>>>>>>>>
>>>>>>>>> We can do that in a followup RFE.  For now, did you see how I did
>>>>>>>>> it for PTR only?
>>>>>>>>>
>>>>>>>>> _STACK_OR_PTR_REG_mask = _PTR_REG_mask;
>>>>>>>>> _STACK_OR_PTR_REG_mask.OR(STACK_OR_STACK_SLOTS_mask());
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> dl
>>>>>>>>>
>>>>>>>>> On 2/18/19 3:43 AM, Roman Kennke wrote:
>>>>>>>>>> I don't know how to do that, though. How should I proceed? The
>>>>>>>>>> issue is
>>>>>>>>>> a nice-to-have for me, but not high-enough priority to dive into
>>>>>>>>>> the
>>>>>>>>>> guts of adlc.
>>>>>>>>>>
>>>>>>>>>> Roman
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> Yes it does, but in this case, only for the PTR class, right?
>>>>>>>>>>> However,
>>>>>>>>>>> it would be nice if
>>>>>>>>>>> adlc would generate it for us.
>>>>>>>>>>>
>>>>>>>>>>> dl
>>>>>>>>>>>
>>>>>>>>>>> On 2/14/19 2:47 AM, Roman Kennke wrote:
>>>>>>>>>>>> It is not so easy.
>>>>>>>>>>>>
>>>>>>>>>>>> We'd also need to generate STACK_OR_$NAME_mask() accessors for
>>>>>>>>>>>> spillable
>>>>>>>>>>>> registers. See RegClass::build_register_masks().
>>>>>>>>>>>>
>>>>>>>>>>>> Roman
>>>>>>>>>>>>
>>>>>>>>>>>>> How about something like this:
>>>>>>>>>>>>>
>>>>>>>>>>>>> http://cr.openjdk.java.net/~dlong//8217909/webrev/
>>>>>>>>>>>>>
>>>>>>>>>>>>> dl
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2/13/19 5:05 AM, Roman Kennke wrote:
>>>>>>>>>>>>>> Apparently, this is a limitation of CodeSnippetRegClass, in
>>>>>>>>>>>>>> formsopt.hpp:
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>        void set_stack_version(bool flag) {
>>>>>>>>>>>>>>          assert(false, "User defined register classes are not
>>>>>>>>>>>>>> allowed to
>>>>>>>>>>>>>> spill to the stack.");
>>>>>>>>>>>>>>        }
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I believe this means I cannot use this approach.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I also don't feel like extending the AD syntax, parser,
>>>>>>>>>>>>>> etc at
>>>>>>>>>>>>>> this
>>>>>>>>>>>>>> point.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Do you have any other suggestions? Or should we go with my
>>>>>>>>>>>>>> original
>>>>>>>>>>>>>> patch?
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Roman
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I started a version based on Dean's approach:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8217909/webrev.01/
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Unfortunately it barks on ptr_reg class:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> assert fails
>>>>>>>>>>>>>>> /home/rkennke/src/openjdk/jdk-jdk/src/hotspot/share/adlc/formsopt.hpp
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> 246: User defined register classes are not allowed to spill
>>>>>>>>>>>>>>> to the
>>>>>>>>>>>>>>> stack.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Interestingly, it doesn't do that when I only handle any_reg
>>>>>>>>>>>>>>> class.
>>>>>>>>>>>>>>> However, I am not sure what that means or how to fix it. Any
>>>>>>>>>>>>>>> hints?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>>> Roman
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> There is a way to do it without cloning all the register
>>>>>>>>>>>>>>>> class
>>>>>>>>>>>>>>>> variants.
>>>>>>>>>>>>>>>> For the arm64 port we massaged the register masks in
>>>>>>>>>>>>>>>> Compile::pd_compiler2_init():
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> http://hg.openjdk.java.net/jdk9/jdk9/hotspot/file/b756e7a2ec33/src/cpu/arm/vm/arm_64.ad#l638
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> It might be worth considering, as the number of variants
>>>>>>>>>>>>>>>> grows.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> dl
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 2/11/19 3:02 AM, Roman Kennke wrote:
>>>>>>>>>>>>>>>>> When running with compressed oops, the r12 register holds
>>>>>>>>>>>>>>>>> the
>>>>>>>>>>>>>>>>> heapbase,
>>>>>>>>>>>>>>>>> and thus is not available to register allocation in C2.
>>>>>>>>>>>>>>>>> However,
>>>>>>>>>>>>>>>>> when
>>>>>>>>>>>>>>>>> *not* running with compressed oops, it is still not
>>>>>>>>>>>>>>>>> available and
>>>>>>>>>>>>>>>>> remains unused. It should be made available to register
>>>>>>>>>>>>>>>>> allocation in
>>>>>>>>>>>>>>>>> this case.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> This patch implements this by introducing with_r12 and
>>>>>>>>>>>>>>>>> no_r12
>>>>>>>>>>>>>>>>> variants
>>>>>>>>>>>>>>>>> of basically all register classes, and add dynamic reg
>>>>>>>>>>>>>>>>> classes to
>>>>>>>>>>>>>>>>> select
>>>>>>>>>>>>>>>>> one or the other, based on current settings. I needed
>>>>>>>>>>>>>>>>> to add
>>>>>>>>>>>>>>>>> UseZGC in
>>>>>>>>>>>>>>>>> those flags because ZGC asserts to not get r12 in its
>>>>>>>>>>>>>>>>> barriers. Not
>>>>>>>>>>>>>>>>> sure
>>>>>>>>>>>>>>>>> that this is necessary.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Note that we might want to use the r12 register in
>>>>>>>>>>>>>>>>> Shenandoah
>>>>>>>>>>>>>>>>> later to
>>>>>>>>>>>>>>>>> keep GC state. In this case, we'd need to add
>>>>>>>>>>>>>>>>> UseShenandoahGC or
>>>>>>>>>>>>>>>>> such to
>>>>>>>>>>>>>>>>> the test. Do we want to abstract this whole check? Not
>>>>>>>>>>>>>>>>> sure
>>>>>>>>>>>>>>>>> that
>>>>>>>>>>>>>>>>> this is
>>>>>>>>>>>>>>>>> possible/feasible to do in .ad though...
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Bug:
>>>>>>>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8217909
>>>>>>>>>>>>>>>>> Webrev:
>>>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~rkennke/JDK-8217909/webrev.00/
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Testing: tier1 no regressions locally, eyeball generated
>>>>>>>>>>>>>>>>> code,
>>>>>>>>>>>>>>>>> yes it
>>>>>>>>>>>>>>>>> does use r12 now.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Can I please get reviews?
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Roman
>>>>>>>>>
>>>>>>>
>>>>
>>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: OpenPGP digital signature
URL: <https://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20190220/25319ff7/signature.asc>