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

[Igor Veresov]

John, thanks for the review!

On Feb 24, 2014, at 12:27 PM, John Rose <john.r.rose at oracle.com> wrote:

> On Feb 23, 2014, at 11:02 PM, Igor Veresov <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.
> 

I assume you’re talking about the first one. The goal of that assert is to basically ensure that br_offs - prev_block_loop_pad doesn’t underflow, which should be true for all the blocks except the first that require padding.

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

I agree, that’ll make the code more robust. I’ll make the changes.

igor

> 
> — John

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20140225/86efe5c5/attachment.html