RFR(M): 8206141: [lworld] Improve accessing a flattened value type array passed as Object[]

[Ioi Lam]

Hi Roland,

I started studying your patch so that I can implement the corresponding 
profiling support inside the interpreter. I have to admit that I don't 
quite understand what's going on :-(, but I have a comment about this 
new block of code in parser2.cpp:


   83     Node* kls = load_object_klass(ary);
   84     Node* lhp = basic_plus_adr(kls, 
in_bytes(Klass::layout_helper_offset()));
   85     Node* layout_val = make_load(NULL, lhp, TypeInt::INT, T_INT, 
MemNode::unordered);
   86     Node* tag = _gvn.transform(new RShiftINode(layout_val, 
intcon(Klass::_lh_array_tag_shift)));
   87     ideal.if_then(tag, BoolTest::ne, 
intcon(Klass::_lh_array_tag_vt_value)); {


which looks the same as this existing block in the same file:

  206         Node* kls = load_object_klass(ary);
  207         Node* lhp = basic_plus_adr(kls, 
in_bytes(Klass::layout_helper_offset()));
  208         Node* layout_val = make_load(NULL, lhp, TypeInt::INT, 
T_INT, MemNode::unordered);
  209         layout_val = _gvn.transform(new RShiftINode(layout_val, 
intcon(Klass::_lh_array_tag_shift)));
  210         ideal.if_then(layout_val, BoolTest::ne, 
intcon(Klass::_lh_array_tag_vt_value)); {


Could the above blocks be somehow refactored, along with the following 
method, so we can avoid duplicated code?


3392 Node* GraphKit::gen_lh_array_test(Node* kls, unsigned int lh_value) {
3393   Node* lhp = basic_plus_adr(kls, 
in_bytes(Klass::layout_helper_offset()));
3394   Node* layout_val = make_load(NULL, lhp, TypeInt::INT, T_INT, 
MemNode::unordered);
3395   layout_val = _gvn.transform(new RShiftINode(layout_val, 
intcon(Klass::_lh_array_tag_shift)));
3396   Node* cmp = _gvn.transform(new CmpINode(layout_val, 
intcon(lh_value)));

Thanks
- Ioi
On 9/27/18 5:35 AM, Roland Westrelin wrote:
> Here is a new webrev for this:
>
> http://cr.openjdk.java.net/~roland/8206141/webrev.01/
>
> that should address the comments from your review and some comments you
> made offline.
>
> The failure you reported happens because an array store is emitted with
> a test for a flattened array and an ArrayCopyNode but after some
> optimizations the type of the array becomes known and the code in
> ArrayCopyNode::Ideal() sees a clone from an instance to an array which
> is unexpected. I added a test case for that and fixed it by bailing out
> of ArrayCopyNode::Ideal() when a mix of array and instance is seen.
>
> I moved the check for a null value from array_store_check() to
> array_store() as you mentioned offline that this would require less
> runtime checks to be executed in some cases.
>
> I also noticed that if we know the type of the value being stored and
> that value cannot be a value type then we're guaranteed the store is to
> a non value array. I changed the logic that emits the runtime check
> accordingly.
>
> Roland.