RFR (M): Add missing compiler checks

[Zoltán Majó]

Hi,


thank you for the review!

On 06/14/2017 01:53 PM, David Simms wrote:
>
> The changes to "src/share/vm/oops/*" and 
> "src/share/vm/interpreter/interpreterRuntime.cpp" look good.
>
> One nit-pick is the use of C cast to "ValueKlass*" rather than 
> "ValueKlass::cast()" like we generally do for Klass casting.

I changed element_klass to have covariant return types (as Tobias 
suggested). That removed the need for ValueKlass::cast()'s.

>
> The extra field in TypeArrayKlass is minimal, given the fixed number 
> of types (even with multi-dim).

OK.

>
> Thanks for the extra klass check in vastore, btw !

Sure!

I'll send the updated code in my reply to Tobias.

Best regards,


Zoltan

>
> Cheers
> /David Simms
>
>
>
> On 14/06/17 12:55, Zoltán Majó wrote:
>> Hi,
>>
>>
>> please review the following change:
>> http://cr.openjdk.java.net/~zmajo/valhalla/04.checks/webrev.00/
>>
>> The change adds some (or hopefully all) checks that C2 currently does 
>> not generate but will most likely be required by the MVT 
>> specification. I tested the change with JPRT (x86_64), no failures 
>> have appeared. I'm currently running the hotspot/compiler JTREG tests 
>> locally.
>>
>> A few notes about the change:
>>
>> - Moving _element_klass from ObjArrayKlass to ArrayKlass is necessary 
>> because that way the klass of the array elements is found at the same 
>> offset in both ObjArrayKlass and ValueArrayKlass instances (and we 
>> need the offset to implement type checks). However, _element_klass is 
>> now present in TypeArrayKlass as well (where it is not needed). Maybe 
>> a better way to organize the class hierarchy would be to put 
>> _element_klass into a new common subclass X of [Obj|Value]ArrayClass; 
>> both X and TypeArrayKlass would then be direct subclasses of 
>> ArrayKlass. Maybe it's better to deal with this aspect once the we're 
>> closer to the final specification/design of MVT.
>>
>> - The change adds a type check to 
>> InterpreterRuntime::value_array_store. I assumed (and expect) vastore 
>> will behave similarly to aastore and requires the element type of 
>> src/dst to match. Please let me know if that is an incorrect assumption.
>>
>> - The bytecodes vdefault/vwithfield can throw an OutOfMemoryError if 
>> no space is available for allocation. With the current C2 
>> implementation allocations can be delayed to a later point in the 
>> program (e.g., when a deoptimization happens). If no free memory is 
>> available at that point to allocate memory for a value type, the OOME 
>> will appear later than the vdefault/vwithfield bytecode that actually 
>> caused it. Tobias and I discussed about this aspect and it's likely 
>> that this is not a problem as re-allocation of scalar-replaced 
>> objects is similar and is also performed at deoptimization. However, 
>> more investigation is needed to confirm that.
>>
>> Thank you!
>>
>> Best regards,
>>
>>
>> Zoltan
>>
>