RFR: 8233506: ZGC: the load for Reference.get() can be converted to a load for strong refs

[Erik Österlund]

Hi Per,

Thanks for the review! Will file a cleanup RFE.

Thanks,
/Erik

> On 8 Nov 2019, at 23:45, Per Liden <per.liden at oracle.com> wrote:
> 
> On 11/7/19 2:49 PM, Erik Österlund wrote:
>> Hi,
>> We have noticed problems with the one single place where we let C2 touch the graph while injecting our load barriers. Right before tagging a load as needing a load barrier, it is GVN transformed. The problem with this is that if we emit a strong field load, the GVN transformation can see through that load, by following it through the store that set its value, and find that the field value really came from the load of a weak Reference.get intrinsic. In this scenario, the load we get out from the GVN transformation needs weak barriers, yet we override it with strong barriers, as that was the semantics of the access being parsed. Sigh. We already have code that tries to determine if the load we got out from the GVN transformation looks like a load that was created in the BarrierSetC2 factory function, so one way of solving this is to refine that logic that tries to determine if this was the load we created before the transformation or not. But I felt like a better solution is to finish constructing the access with all the intended properties *before* transformation.
>> I massaged the code so that the GC barrier data of accesses with load barriers gets passed in to the factory functions that create the access, right before the transformation. This way, we construct the access with the intended semantics where it is being created (parser or macro expansion for field accesses in clone intrinsics). Then we do not have to touch it after the GVN transformation.
>> It does seem like there could be similar problems from other GCs, but in e.g. G1, the consequences are weird suboptimal code instead of anything dangerous happening. For example, we can generate SATB buffering code required by G1 Reference.get() intrinsics for strong accesses, due to GVN handing out earlier accesses with different semantics. Perhaps that should be looked into separately as well. But that investigation is outside of the scope of this bug fix.
>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8233506/webrev.00/
> 
> Looks good!
> 
> As discussed off-line, even if it's not a problem for ZGC, we should fix so that we never call access.set_raw_access() *after* GVN transformation. But let's do that as a separate fix.
> 
> /Per
> 
>> Bug:
>> https://bugs.openjdk.java.net/browse/JDK-8233506
>> Thanks,
>> /Erik