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

[erik.osterlund at oracle.com]

Hi Tobias,

On 11/8/19 9:38 AM, Tobias Hartmann wrote:
> 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?

Nope! :) Because the access nodes have their barrier data populated 
before transformation, describing the semantics of the produced access, 
we don't care what GVN gives us after transformation. Whatever it gives 
us has the correct semantics for the corresponding access that produced it.

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

Sure, will file another RFE for that.
Regarding the assert, it's not obvious what it would look like, since 
the assert in the transformation code has to know exactly what nodes are 
expected to have GC data. For example, a LoadP might be used to read any 
pointer, including but not limited to oops. And some oops don't need 
barriers like the threadOop due to being processed in safepoints. So 
given a LoadP node for example, I don't know if we can determine whether 
it should or should not have GC data.

>> Webrev:
>> http://cr.openjdk.java.net/~eosterlund/8233506/webrev.00/
> Looks good to me!

Thanks Tobias!

/Erik

> Best regards,
> Tobias