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

[Vladimir Kozlov]

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
>