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

[Vladimir Kozlov]

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