RFR(S): 8035283 Second phase of branch shortening doesn't account for loop alignment
igor.veresov at oracle.com
Tue Feb 25 09:55:55 PST 2014
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.
> — John
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-compiler-dev