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

[Igor Veresov]

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