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

[Igor Veresov]

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
>>