[aarch64-port-dev ] RFR: Shenandoah import

[Roman Kennke]

Am 04.10.2017 um 12:23 schrieb Aleksey Shipilev:
> On 10/04/2017 12:12 PM, Andrew Haley wrote:
>> Some minor stuff:
>>
>> --- a/src/share/vm/opto/macro.cpp
>> +++ b/src/share/vm/opto/macro.cpp
>> @@ -624,7 +624,7 @@
>>                                      k < kmax && can_eliminate; k++) {
>>             Node* n = use->fast_out(k);
>>             if (!n->is_Store() && n->Opcode() != Op_CastP2X &&
>> -              !n->is_g1_wb_pre_call()) {
>> +              (!UseShenandoahGC || !n->is_g1_wb_pre_call())) {
>>
>> This logic is super-confusing.  It could be
>>
>>             if (! UseShenandoahGC &&
>>                 !n->is_Store() && n->Opcode() != Op_CastP2X &&
>>                 !n->is_g1_wb_pre_call()) {
>>
>> it could even be
>>
>>             if (! UseShenandoahGC &&
>>                 ! (n->is_Store() || n->Opcode() == Op_CastP2X ||
>>                    n->is_g1_wb_pre_call()) {
>>
>> but I guess that'd be a divergence from upstream, which we want to
>> minimize.  in general, though, for things that we add we should reduce
>> the complexity of boolean operations where it makes sense.
> Yes, and I think the suggestions are silently breaking: they expose "n->is_g1_wb_pre_call()" when
> UseShenandoahGC=false.
>
> The actual difference against upstream after this patch is this:
>
> -          if (!n->is_Store() && n->Opcode() != Op_CastP2X) {
> +          if (!n->is_Store() && n->Opcode() != Op_CastP2X &&
> +              (!UseShenandoahGC || !n->is_g1_wb_pre_call())) {
>
> https://builds.shipilev.net/patch-openjdk-shenandoah-jdk8/2017-10-04-v41-vs-20d83f8419c4/src/share/vm/opto/macro.cpp.sdiff.html
>
> I think it is a good style to make the "&& (expr)" additions to predicates where expr clearly
> coverts itself into "true" when UseShenandoahGC is "false".
I would go even further and use nested if statements like:
if (UseShenandoahGC) {
   if (blabla) {
   }
}

or

if (!UseShenandoahGC) {
if (blabla) {
}
}

and even retain intendation of upstream-code... this way it is 1. 
super-clear what is Shenandoah only or non-Shenandoah code, and 2. 
trivial to see in a diff too. We must be very careful to not change 
evaluation ordering on non-Shenandoah paths.