[9 or 10?] RFR(M): 8176506: C2: loop unswitching and unsafe accesses cause crash

[Vladimir Ivanov]

>>> Isn't the plan to remove the membars for 9 and put it back in 10?
>>
>> JDK-8176513 removed membars around (possibly) mixed accesses and the fix
>> we are discussing makes them obsolete.
>
> If we use unsafe to access a heap allocated object but it's impossible
> for the compiler to tell it's heap allocated, then we would compile it
> as a raw memory access. If we access the same object's field with a
> regular access in the same method, then there's a chance that access
> would be reordered with the unsafe access. So we still need the membars?

Yes, my bad.

>> My last concern is that by marking possibly mixed accesses as RAW during
>> parsing, we limit what optimizations can be applied later.
>>
>> I thought about allowing rawptr => oopptr conversions for mixed accesses
>> once they become on-heap (+ some cleanups; full patch [1]):
>>
>> http://cr.openjdk.java.net/~vlivanov/roland/8176506/webrev.01_00/
>>
>> What do you think about it?
>
> Thanks for the cleanup and fixes.
> Isn't converting rawptr => oopptr a variation of:
>
> http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/2016-April/022745.html
>
> that Vladimir K didn't like?
>
> Uses of the AddP records the memory slice they operate on:
> MemNode::_adr_type, LoadStoreNode::_adr_type. That would need to be
> fixed if the AddP type changes to avoid inconsistencies.
>
> It seems unlikely but if the raw memory type was captured by a node: a
> Phi that would merge 2 AddPNodes for instance. If the AddPNode types
> change to oop ptr, then the Phi type wouldn't be oop ptr. I wonder if
> that could cause problems.

Good point. I erroneously assumed that the difference between (rawptr => 
oopptr) and (oopptr => rawptr) conversions is widening vs narrowing 
types, but it's not the case since rawptr & oopptr types are disjoint.

So, the proper type for mixed accesses would be TypePtr::BOTTOM, but it 
won't make the compiler happy :-)

> If we think the compiler would find more non null bases after some
> optimizations than it does at parse time, then one solution could be to
> delay inlining of the unsafe accesses.

Ok, let's leave mixed accesses aside. If it turns out to be an important 
case, then we can revisit it later. Argument profiling should minimize 
the chances of on-heap unsafe access to be treated as mixed.

>> Also, I think the checks in LoadNode::split_through_phi &
>> MemNode::Ideal_common I mentioned before affect normal accesses as well,
>> so probably the problem with loop unswitching you encountered isn't
>> specific to unsafe accesses. But let's keep it separate.
>
> Can you point to the exact code you're referring to?

http://hg.openjdk.java.net/jdk9/jdk9/hotspot/file/78c27c5148a7/src/share/vm/opto/memnode.cpp#l1312

http://hg.openjdk.java.net/jdk9/jdk9/hotspot/file/78c27c5148a7/src/share/vm/opto/memnode.cpp#l331

Best regards,
Vladimir Ivanov