RFR(S) 8231586: enlarge encoding space for OopMapValue offsets

[dean.long at oracle.com]

Looks great!

dl

On 10/8/19 12:42 PM, Tom Rodriguez wrote:
>
>
> dean.long at oracle.com wrote on 10/7/19 10:43 PM:
>> Looks good.  The only thing I noticed is that while you moved the 
>> asserts into the OopMapValue ctor, the read_from method still 
>> bypasses those asserts.  Would it make sense to move those asserts 
>> into set_content_reg()?
>
> Yes.  I've moved them and the new webrev is 
> http://cr.openjdk.java.net/~never/8231586.2/webrev.  I've submitted a 
> new mach5 job.
>
> tom
>
>>
>> dl
>>
>> On 10/7/19 10:11 AM, Tom Rodriguez wrote:
>>>
>>>
>>> dean.long at oracle.com wrote on 10/5/19 9:25 PM:
>>>> You're right, my mistake.  Your change looks good.  I was actually 
>>>> looking at the set_* methods in OopMapValue, not OopMap, and 
>>>> incorrectly thought the | would preserve the previous value.  On 
>>>> closer inspection, it doesn't, and those methods don't even get 
>>>> called, because the ctor uses a different set method.  Can we 
>>>> remove these unused methods?
>>>
>>> Good idea.  I've deleted those along with some other dead methods 
>>> and rearranged the code a little bit to hide more of the API so it's 
>>> clearer what might be done to an OopMapValue. The delta webrev is 
>>> just my changes and the .1 webrev is the full new webrev.  I'm 
>>> submitting a mach5 run now.
>>>
>>> http://cr.openjdk.java.net/~never/8231586-delta/webrev/
>>> http://cr.openjdk.java.net/~never/8231586.1/webrev/
>>>
>>> tom
>>>
>>>>
>>>>      void set_oop()              { set_value((value() & 
>>>> register_mask_in_place) | oop_value); }
>>>>      void set_narrowoop()          { set_value((value() & 
>>>> register_mask_in_place) | narrowoop_value); }
>>>>      void set_callee_saved()     { set_value((value() & 
>>>> register_mask_in_place) | callee_saved_value); }
>>>>      void set_derived_oop()      { set_value((value() & 
>>>> register_mask_in_place) | derived_oop_value); }
>>>>
>>>> dl
>>>>
>>>> On 10/5/19 10:41 AM, Tom Rodriguez wrote:
>>>>>
>>>>>
>>>>> dean.long at oracle.com wrote on 10/4/19 4:27 PM:
>>>>>> It's not obvious that we only set 1 bit. The set methods don't 
>>>>>> enforce that.  And this code looks like it is setting both 
>>>>>> "derived" and "oop":
>>>>>
>>>>> Each of those calls produce independent OopMapValues. 
>>>>> http://hg.openjdk.java.net/jdk/jdk/file/ff45c1bf8129/src/hotspot/share/compiler/oopMap.cpp#l137 
>>>>>  Also all tests against the type are equality tests, not bitmasks, 
>>>>> so if more than one bit was set it would fail to match anything. 
>>>>> The bitmask-ness of the values is only used for filtering the 
>>>>> iteration.
>>>>>
>>>>> tom
>>>>>
>>>>>>
>>>>>> http://hg.openjdk.java.net/jdk/jdk/file/ff45c1bf8129/src/hotspot/share/opto/buildOopMap.cpp#l315 
>>>>>>
>>>>>>
>>>>>> dl
>>>>>>
>>>>>> On 10/4/19 10:26 AM, Tom Rodriguez wrote:
>>>>>>> http://cr.openjdk.java.net/~never/8231586/webrev
>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8231586
>>>>>>>
>>>>>>> The current OopMapValue encoding uses a bit mask for each value 
>>>>>>> even though only one bit is ever set. Since only 16 bits are 
>>>>>>> available for encoding this limits the offsets it can express. 
>>>>>>> Compilation with a large number of stack slots can bailout 
>>>>>>> because of this limit.  This changes the encoding to use 2 bits 
>>>>>>> which gives 2 bits back to the offset.
>>>>>>>
>>>>>>> I also deleted some StressDerivedPointers machinery that's been 
>>>>>>> completely unimplemented for years (decades?).  The flag itself 
>>>>>>> is now dead but I wasn't sure if there are test references to it 
>>>>>>> somewhere. Should I delete the flag as well?
>>>>>>>
>>>>>>> mach5 testing is in progress.
>>>>>>>
>>>>>>> tom
>>>>>>
>>>>
>>