RFR (M): Add missing compiler checks

Zoltán Majó zoltan.majo at oracle.com
Wed Jun 14 15:37:04 UTC 2017


Hi Tobias,


thank you for the review!

On 06/14/2017 02:10 PM, Tobias Hartmann wrote:
> Hi Zoltan,
>
> nice work!

Thank you.

>
> Here are some comments:
> - Why are the changes to bcEscapeAnalyzer.cpp necessary?

 From a functional point of view, that change is not strictly necessary 
because T_VALUETYPE==T_OBJECT (that's the only difference between the 
handling of aastore and vastore). But maybe it's good to not use OBJECT 
in the code when we actually refer to a VT. I can also remove that 
change, if you want.
> - I don't like all the ValueKlass* casts. Couldn't we avoid them by making element_klass() virtual and returning ValueKlass* from ValueArrayKlass::element_klass() (covariant return)? Like this, we could also avoid the asserts in valueArrayKlass.hpp.

Thank you -- I did that and the code got simpler.

> - In parseHelper.cpp, why do we need a runtime check if we statically know that target_elem_klass != source_klass?

The two types may be unequal at compilation type but they can be equal 
at runtime.

> - I think it would be good to add a test that mixes value type arrays with non value type stores (and the other way around)


I tried (see, e.g., test85() below). Unfortunately, compilation fails. 
What I'm not sure about is whether the verifier should (and can) catch 
such issues or should we really handle it in the compiler.

> - Please add a comment to test83 and test84

I did.

Here is the updated code:
http://cr.openjdk.java.net/~zmajo/valhalla/04.checks/webrev.01/

Testing is in progress.

Best regards,


Zoltan

P.S.: Here is test85 I mentioned before:
http://cr.openjdk.java.net/~zmajo/valhalla/04.checks/test85



>
> Thanks,
> Tobias
>
> On 14.06.2017 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
>>



More information about the valhalla-dev mailing list