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

[Vladimir Kozlov]

Good!

Thanks,
Vladimir

On 2/25/14 11:05 AM, Igor Veresov wrote:
> Ok, sure.
>
> Updated webrev: http://cr.openjdk.java.net/~iveresov/8035283/webrev.03/
>
> igor
>
> On Feb 25, 2014, at 10:10 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>
>> I don't see the reason to declare array as unsigned int[]. You store and load int values.
>>
>> Thanks,
>> Vladimir
>>
>> On 2/25/14 9:57 AM, Igor Veresov wrote:
>>> Vladimir, that for the suggestion.
>>>
>>> Here the new version of the change: http://cr.openjdk.java.net/~iveresov/8035283/webrev.02/
>>>
>>> Thanks!
>>> igor
>>>
>>> On Feb 24, 2014, at 2:55 PM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
>>>
>>>> After discussing with John I agree with him. From current code it is not obvious that code at the line #448:
>>>>
>>>> 448       int max_loop_pad = nb->code_alignment()-relocInfo::addr_unit();
>>>>
>>>> produces the same value as new code at the line #505:
>>>>
>>>> 505         int prev_block_loop_pad = block->code_alignment() - relocInfo::addr_unit();
>>>>
>>>> because code_alignment() is not one-liner.
>>>>
>>>> I am fine with adding additional array block_worst_case_pad[]:
>>>>
>>>>        int max_loop_pad = nb->code_alignment()-relocInfo::addr_unit();
>>>> +      block_worst_case_pad[i+1] = max_loop_pad;
>>>>        if (max_loop_pad > 0) {
>>>>
>>>> Also prev_block_loop_pad name in new code is confusing. It is padding for current block even so the padding is inserted in previous block.
>>>>
>>>> +         // This block may need special alignment, account for
>>>> +         // the padding before it.
>>>> +         int block_padding = block_worst_case_pad[i];
>>>> +         if (i > 0 && block_padding > 0) {
>>>> +           assert(br_offs >= block_padding, "Should have at least a padding on top");
>>>> +         } else {
>>>> +           // First block or not a loop
>>>> +           block_padding = 0;
>>>> +         }
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>> On 2/24/14 12:27 PM, John Rose wrote:
>>>>> On Feb 23, 2014, at 11:02 PM, Igor Veresov <igor.veresov at oracle.com
>>>>> <mailto:igor.veresov at oracle.com>> wrote:
>>>>>
>>>>>> May I please have a second review of this?
>>>>>> Webrev:http://cr.openjdk.java.net/~iveresov/8035283/webrev.01/
>>>>>
>>>>> I don't understand the force of the assert; it seems to be true mostly
>>>>> by accident.
>>>>>
>>>>> Maybe you want an assert that 'last_may_be_short_branch_adr' does not
>>>>> fall between (br_offs - prev_block_loop_pad)+1 and br_offs, inclusive?
>>>>>
>>>>> It took me a long time to convince myself that moving the goalpost for
>>>>> the comparison to 'last_may_be_short_branch_adr' was safe.  Really, the
>>>>> argument hinges on the fact that all layout info. is relative to a
>>>>> pessimistic assumption that the maximum possible padding
>>>>> (block->code_alignment() - relocInfo::addr_unit()) is always inserted.
>>>>>
>>>>> I suggest making the linkage to that assumption clearer, by hoisting the
>>>>> crucial expression 'block->code_alignment() - relocInfo::addr_unit()' as
>>>>> follows:
>>>>>
>>>>>    uint*      worst_case_pad  = NEW_RESOURCE_ARRAY(uint,nblocks);
>>>>> ...
>>>>>
>>>>>    worst_case_pad[i] = block->code_alignment() - relocInfo::addr_unit();
>>>>>
>>>>> Then use the array reference directly instead of the now-linked uses of
>>>>> code_alignment etc.
>>>>>
>>>>> This is delicate code!
>>>>>
>>>>> — John
>>>
>