RFR (M): Add missing compiler checks

[Tobias Hartmann]

Hi Zoltan,

nice work! 

Here are some comments:
- Why are the changes to bcEscapeAnalyzer.cpp necessary?
- 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.
- In parseHelper.cpp, why do we need a runtime check if we statically know that target_elem_klass != source_klass?
- 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)
- Please add a comment to test83 and test84

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
>