RFR(L): 8203197: C2: consider all paths in loop body for loop predication

[Vladimir Kozlov]

On 6/1/18 7:58 AM, Roland Westrelin wrote:
> 
> Hi Vladimir,
> 
> Thanks for reviewing this.
> 
>> I think you should make new methodData layout as separate change. I want
>> it to be pushed and tested separately to make sure it does not cause any
>> issues.
>> I am fine with your approach. Few comments:
>>
>> +#ifndef _LP64
>> +    speculative_trap_padding,
>> +#endif
>>
>> why you need it?
> 
> MethodData::bci_to_extra_data() makes assumption about the size of a
> SpeculativeTrapData. See assert there. If it doesn't hold I think that 
> code is broken.


SpeculativeTrapData has only one cell for Method* which should be only 
one cell, even in 32-bit VM. Change it to 2 cells is wrong I think.

I think old assumption is invalid with your changes:

// This code assumes an entry for a SpeculativeTrapData is 2 cells

because you changed header size for 32 bit - it is now 2 cells.

It may actually cause issues (with 32-bit VM) in other places with 
similar assumption.

An other issue is that header can't be allocated/set/load atomically in 
32-bit VM as MethodData::bci_to_extra_data_helper() expecting.

Please, evaluate these issues.

> 
>> In c1_LIRAssembler_aarch64.cpp you replaced LogBytesPerWord with 0
>> except last case. Why keep it in last case?
> 
> That must be wrong. I'll double check that.
> 
>> No changes for SPARC, PPC64, etc?
> 
> Sparc should be ok from code inspection but I can't test it so I will
> need someone else to. I intended to ask for help for PPC64 etc. but I
> wanted to first get that review started and see if there were
> objections.

Okay.

> 
>> Did you take into account the dominating (loop exit) check when you move
>> a following check as profiling predicate? There could be dependencies
>> between them (klass check, for example, before field load and then NULL
>> check you want to move).
> 
> I don't understand what you are referring to with loop exit.
> There can be dependencies between predicates as you say but they are
> kept in the order they are moved out of loop so dependent predicates are
> always executed after the ones they depend on.

Then I don't understand your changes. I thought you move out checks (as 
new predicates) which have profiling information which means you have an 
other valid path branch from dominating check. Such check can't be moved 
from loop as I remember. I don't think you can move out from loop loop 
exit checks.

Please, explain which checks you moved from loop as profiling predicates 
without moving dominating checks.


> 
>> Why Shenandoah's barriers checks do not convert into implicit NULL checks?
> 
> It does. But still we want the barriers hoisted out of loop whenever
> possible. If the null check stays in the loop then it keeps the barrier
> in the loop as well.

Got it.

Thanks,
Vladimir


> 
> Roland.
>