[aarch64-port-dev ] RFR: 8139674: aarch64: guarantee failure in TestOptionsWithRanges.java

[Vladimir Kozlov]

On 10/16/15 5:12 PM, Edward Nevill wrote:
> Hi Vladimir,
>
> Thanks for the review.
>
> On Fri, 2015-10-16 at 08:53 +0800, Vladimir Kozlov wrote:
>> I don't think removing next line is right:
>>
>> -        add(mdp, mdp, to_add);
>>
>> it shifts to next metadata cell.
>
> I believe removal of this line is correct. The old code kept mdp pointing at the current metadata cell that it was processing. This is what lead to the large negative offsets when it was trying to index the two fields at the start of the metadata structure.
>
> So in the old code, to process each metatdata cell it did.
>
>      Address mdo_arg_addr(mdp, in_bytes(TypeEntriesAtCall::argument_type_offset(i))-off_to_args);
>      profile_obj_type(tmp, mdo_arg_addr);
>
> In the revised code it does
>
>      Address mdo_arg_addr(mdp, in_bytes(TypeEntriesAtCall::argument_type_offset(i)));
>      profile_obj_type(tmp, mdo_arg_addr);
>
> 'off_to_args' is updated by 'to_add' each time round the loop.
>
>      off_to_args += to_add;
>
> So in the old code the address of the metadata cell was calculated as
>
>      mdp + in_bytes(TypeEntriesAtCall::argument_type_offset(i))-off_to_args
>
> Note: 'in_bytes(TypeEntriesAtCall::argument_type_offset(i))-off_to_args' always evaluates to 0!
>
> In the revised code the address is calculated as 'in_bytes(TypeEntriesAtCall::argument_type_offset(i))' which evaluates to the same address since mdp is no longer offset by 'off_to_args'.

Make sense. Thank you for explanation.

>
>>
>> Also both exit paths execute the same instruction:
>>
>> +      add(rscratch1, mdp, off_to_args);
>>
>> may be you can simplify exit without them:
>>
>>          bind(done);
>> +      add(mdp, mdp, off_to_args);
>
> I don't believe the exit can be simplified.
>
> The reason is that 'off_to_args' is different for each iteration around the loop. So in assembler what we will have is
>
>      add   x8, x0, #8 // x0 == mdp, offset to arg 0 == 8
>      blt   done
>      ...
>      add   x8, x0, #0x18 // arg 1 offset = 0x18
>      blt   done
>      ....
>
>      add   x8, x0, #0x108 // fallthru case
> done:
>      mov   x0, x8
>
> So, mdp is updated by a different immediate value in each case, so we cannot merge them into a single case

Okay, I missed that off_to_args is changing.

Looks good.

Thanks,
Vladimir

>
> done:
>      add   x0, x0, #N  // N == ???
>
> Thanks for your help,
> Ed.
>
>