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

[Tom Rodriguez]

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