[15] RFR(S): 8235332: TestInstanceCloneAsLoadsStores.java fails with -XX:+StressGCM

[Christian Hagedorn]

Hi Nils

Thank you for the review!

Best regards,
Christian

On 30.01.20 11:41, Nils Eliasson wrote:
> Hi Christian,
> 
> Change looks good,
> 
> Thanks for fixing!
> 
> // Nils
> 
> On 2020-01-15 12:02, Christian Hagedorn wrote:
>>>>> If there's no GC barrier then:
>>>>>
>>>>> Node* control_proj_ac = bs->step_over_gc_barrier(proj_in->in(0));
>>>>>
>>>>> control_proj_ac is not a projection, it's proj_in->in(0) so the test
>>>>> below can never succeed?
>>>>
>>>> If there is no GC barrier, then proj_in is a membar with its control
>>>> input being a control projection from the ArrayCopyNode.
>>>>
>>>> As an example, I have taken the test case TestEliminatedCloneBadMemEdge
>>>> [1] which does not disable ReduceInitialCardMarks and has no GC 
>>>> barrier.
>>>> We have the following situation at [2]:
>>>>
>>>> proj_in is 132 MemBarCPUOrder. The original code then looked at the
>>>> MergeMem 100 input following in(Compile::AliasIdxRaw) which is 101 Proj
>>>> and its input 98 ArrayCopy. This passes the check.
>>>>
>>>> My fix now directly looks at the control input of 132 MemBarCPUOrder
>>>> which is the projection 99 Proj coming from the 98 ArrayCopy. This also
>>>> passes the check.
>>>
>>> Ah! Right. I wonder if there's a reason why the current code follows the
>>> control graph (as you do) instead of the memory graph. I couldn't find
>>> one and your change looks good to me.
>>
>> Thank you for your review Roland! Was wondering about that, too.
>>
>> Best regards,
>> Christian
>