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

[Vladimir Ivanov]

> http://cr.openjdk.java.net/~rkennke/JDK-8217909/webrev.03/
> 
> Reviews? Opinions?

I like it.

Do you need anything special for 32-bit x86 port in x86_32.ad?

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