(original) (raw)

John, thanks for the review!

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

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