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

Wed Feb 20 09:45:14 UTC 2019

Thanks, Nils!

I'll push it through jdk/submit and then push it to jdk/jdk.

Thanks,
Roman

> I like it - much better that the exponential growth of reg_classes. And
> we loose a branch on every access of a dynamic reg_classes. The
> reg_mask_init gets a bit dense, but I can live with that. It is at least
> obvious what it does.
> 
> Reviewed,
> 
> Thanks!
> 
> Nils
> 
> 
> On 2019-02-19 13:14, Roman Kennke wrote:
>> And here comes the 'final' version:
>>
>> Incremental (vs. -02):
>> http://cr.openjdk.java.net/~rkennke/JDK-8217909/webrev.03.diff/
>> Full webrev:
>> http://cr.openjdk.java.net/~rkennke/JDK-8217909/webrev.03/
>>
>> Reviews? Opinions?
>>
>> Thanks,
>> Roman
>>
>>
>>> 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/690f37d8/signature.asc>