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

[Per Liden]

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