RFR (S): 8012715: G1: Fix bug in graphKit.cpp accessing PtrQueue::_index

[Doerr, Martin]

Hi John,

thanks for your input. Here's the new webrev:
http://cr.openjdk.java.net/~goetz/webrevs/8012715/

"zero" and "zeroX" are only used once. Would it be better to remove these local variables and put "ConI" and "ConX" to where they are used?

You have also asked about C1. I had also checked SPARC which looks ok.  I agree with that it would be better to also fix C1 on x64_64.
However, we are not using C1, so I'd prefer if you could handle it in a separate CR.

Kind regards,
Martin


From: John Cuthbertson [mailto:john.cuthbertson at oracle.com]
Sent: Dienstag, 23. April 2013 19:32
To: Doerr, Martin
Cc: hotspot-compiler-dev at openjdk.java.net; Mikael Gerdin; Lindenmaier, Goetz
Subject: Re: RFR (S): 8012715: G1: Fix bug in graphKit.cpp accessing PtrQueue::_index

Hi Martin,

Overall the changes look good to me. I kind of agree with Christian though and I don't like the addition of T_X as an alias for a BasicType. I would prefer something like:

  __ if_then(marking, BoolTest::ne, zero); {
    BasicType index_bt = TypeX_X->basic_type();
    assert(sizeof(size_t) == type2aelembytes(index_bt), "Loading G1 PtrQueue::_index with wrong size.");
    Node* index   = __ load(__ ctrl(), index_adr, TypeX_X, index_bt, Compile::AliasIdxRaw);

i.e. using the basic_type() routine in TypeLong or TypeInt.

Minor nit:
* Instead of __ ConX(0), can you define a zeroX and use that instead? That will make:

      // is the queue for this thread full?
      __ if_then(index, BoolTest::ne, zeroX, likely); {

look more consistent with:

  // if (!marking)
  __ if_then(marking, BoolTest::ne, zero); {

Thanks,

JohnC
On 4/19/2013 5:02 AM, Doerr, Martin wrote:
Hi all,

we found a bug in the G1 barriers generated by the C2 compiler.

In graphKit INT operations were generated to access PtrQueue::_index which
has type size_t. This is 64 bit on 64-bit machines. No problems occur on
little endian machines as long as the index fits into 32 bit, but on
big endian machines the upper part is read, which is zero. This leads
to unnecessary branches to the slow path into the runtime.

The fix introduces X operations where INT was used:
http://cr.openjdk.java.net/~goetz/webrevs/g1-size_t_bug/<http://cr.openjdk.java.net/%7Egoetz/webrevs/g1-size_t_bug/>

This also removes a cast node.

We have also added a type T_X in globalDefinitions.hpp. Is there
already a mechanism to express this?

Please supply a bug id and review this change.

Best regards,
Martin


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