RFR: 8227407: ZGC: C2 loads and load barriers can get separated by safepoints

[Erik Österlund]

Hi Roland,

I see your point. The reason I went with the existing Pinned is the 
following commentary in LoadNode::depends_only_on_test():

   // depends_only_on_test is almost always true, and needs to be almost 
always
   // true to enable key hoisting & commoning optimizations. However, 
for the
   // special case of RawPtr loads from TLS top & end, and other loads 
performed by
   // GC barriers, the control edge carries the dependence preventing 
hoisting past
   // a Safepoint instead of the memory edge.

This comment seemed to suggest that if depends_only_on_test() is false, 
it's not allowed to
move such loads past safepoints, because they could be GC related. So 
the Ideal transformation
that moves the load past safepoints without checking if they 
depends_only_on_test() or not
would be invalid, e.g. for:
   - G1's (or Shenandoah's) "is active" check for SATB buffers.
   - ZGC bad mask
   - G1/Shenandoah barrier queues
   - CondCardMark loads for CMS/Parallel/Serial
   - TLAB bounds checks, and top pointer

Maybe we should look at this the other way around - that Unsafe loads 
are the special ones here, by
being !depends_only_on_test() but still allowing floating past 
safepoints, unlike all the other !depends_only_on_test() loads,
and hence being "mostly pinned".

How would you feel about adding a new enum value for UnsafePinned, and 
modifying the LoadNode::Ideal transformation to
explicitly allow the floating if the !depends_only_on_test() || 
is_unsafe_pinned()?

The load barriers would promote the the pinning from UnsafePinned to 
Pinned, and hence not allow the transformation.

Thanks,
/Erik

On 2019-07-22 11:15, Roland Westrelin wrote:
> Hi Erik,
>
>> http://cr.openjdk.java.net/~eosterlund/8227407/webrev.02/
> Pinned was initially for unsafe accesses where a load can implicitly
> depend on some condition to be valid and shouldn't float above that
> condition. In that use case stepping over a safepoint is ok. So I wonder
> if it would make sense to extend ControlDependency with a new element
> for your use case which is somewhat different instead of overloading the
> meaning of Pinned.
>
> Roland.