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

[Nils Eliasson]

Hi Erik,

Looks good!

Regards,

Nils

On 2019-11-07 14:49, 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/
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8233506
>
> Thanks,
> /Erik