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

[Tobias Hartmann]

Hi Erik,

On 07.11.19 14:49, Erik Österlund wrote:
> 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. 

Do we still need that logic with your change?

> But I felt like a better
> solution is to finish constructing the access with all the intended properties *before* transformation
Yes, that's much better.

> 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.

Could you please file an RFE for that? I also wonder if we could assert that the barrier data is set
when GVN is performed? That way we would catch problems like the one you've described above early.

> Webrev:
> http://cr.openjdk.java.net/~eosterlund/8233506/webrev.00/

Looks good to me!

Best regards,
Tobias