[8u] RFR: 8233023: assert(Opcode() == mem->Opcode() || phase->C->get_alias_index(adr_type()) == Compile::AliasIdxRaw) failed: no mismatched stores, except on raw memory

[Severin Gehwolf]

Hi Roland,

On Tue, 2019-11-05 at 14:42 +0100, Roland Westrelin wrote:
> Hi Severin,
> 
> Thanks for taking care of this.

Thanks for the review!

> > Could I please get a review of this 8u only issue? The reason a
> > fastdebug build of latest OpenJDK 8u asserts for the dec-tree benchmark
> > of the renaissance suite is because the 8u backport of JDK-8140309 was
> > missing this hunk from JDK 9[1]:
> > 
> > +             (Opcode() == Op_StoreL && st->Opcode() == Op_StoreI) || // expanded ClearArrayNode
> > +             (is_mismatched_access() || st->as_Store()->is_mismatched_access()),
> > 
> > I had a closer look and there doesn't seem to be missing anything else.
> > The proposed fix is to amend the assert condition in the appropriate
> > place, which brings 8u in line with JDK 9 code where the failure isn't
> > observed.
> > 
> > Bug: https://bugs.openjdk.java.net/browse/JDK-8233023
> > webrev: http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233023/01/webrev/
> 
> Isn't this:
> 
> @@ -3213,6 +3221,9 @@
>  // within the initialized memory.
>  intptr_t InitializeNode::can_capture_store(StoreNode* st, PhaseTransform* phase, bool can_reshape) {
>    const int FAIL = 0;
> +  if (st->is_unaligned_access()) {
> +    return FAIL;
> +  }
>    if (st->req() != MemNode::ValueIn + 1)
>      return FAIL;                // an inscrutable StoreNode (card mark?)
>    Node* ctl = st->in(MemNode::Control);
> 
> also missing from the 8140309?

It wasn't missing from the 8u backport of 8140309. See:
http://hg.openjdk.java.net/jdk8u/jdk8u/hotspot/rev/0ffee573412b

It was removed later by JDK-8202414.

> It must be armless because nothing sets _unaligned_access AFAICT but
> given the unaligned access part of the patch was backported I think we
> should keep it consistent.
> 
> Also, 
> 
> +             (Opcode() == Op_StoreL && st->Opcode() == Op_StoreI) || // expanded ClearArrayNode
> 
> is not from 8140309 but from 8080289.

Aah, good catch. I didn't mean to include part of 8080289. Updated
webrev:
http://cr.openjdk.java.net/~sgehwolf/webrevs/JDK-8233023/02/webrev/

> We're not going to backport it and
> that line is unrelated to the change so backporting it sounds good. But
> while doing this, we should backport the other changes to that assert
> from 8080289 as well.
> 
> +             st->Opcode() == Op_StoreVector ||
> +             Opcode() == Op_StoreVector ||

Right. I've opted for not including any parts of 8080289, so we should
be consistent.

Thoughts?

Thanks,
Severin