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

[dean.long at oracle.com]

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()?

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