RFR(S): 8035283 Second phase of branch shortening doesn't account for loop alignment

[Igor Veresov]

Thanks for the review, Vladimir!

igor

On Feb 20, 2014, at 4:03 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:

> Yes, you are right about check in assert. Changes are good.
> 
> Thanks,
> Vladimir
> 
> On 2/20/14 4:01 PM, Igor Veresov wrote:
>> 
>> On Feb 20, 2014, at 2:59 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>> 
>>> I think I restored my knowledge of this code :)
>>> 
>>> My main concern was about validity of next expression during all iterations done in phase 2:
>>> 
>>> bool needs_padding = ((uint)(br_offs - prev_block_loop_pad) == last_may_be_short_branch_adr);
>>> 
>>> And it seems right because we don't change default loop padding during phase 2. We only adjust blocks offsets on difference between long and short branch instructions sizes.
>>> 
>>> What confused me that from this code it is not clear that it can happen only for branches at the beginning of block.
>>> 
>>> May be we should add assert:
>>> 
>>> assert(needs_padding == (jmp_offset[i] == 0), "padding only branches at the beginning of block”);
>> 
>> I think it should be more like:
>> assert(!needs_padding || jmp_offset[i] == 0, "padding only branches at the beginning of block”);
>> Because you don’t always need additional padding, even if the only thing that is the block is jump. The padding kicks in only if we changed the branch in the previous block. But if it does kick in, the offset of the jump should indeed be 0.
>> 
>> Updated the webrev:  http://cr.openjdk.java.net/~iveresov/8035283/webrev.01/
>> Retested with CTW.
>> 
>> igor
>> 
>>> 
>>> Thanks,
>>> Vladimir
>>> 
>>> On 2/19/14 6:10 PM, Igor Veresov wrote:
>>>> Branch shortening should take care of the case when a there is an “avoid-back-to-back” instruction in the loop header preceded by the block with another “avoid-back-to-back” instruction with the loop padding. The machinery did not account for the fact the padding can be completely eliminated during the emission.
>>>> 
>>>> Webrev: http://cr.openjdk.java.net/~iveresov/8035283/webrev.00/
>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8035283
>>>> 
>>>> Testing: the affected application, CTW, jtreg (on SPARC-T4 with loop alignment 4 and 16), jprt
>>>> 
>>>> Thanks!
>>>> igor
>>>> 
>>