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

[John Cuthbertson]

Hi Martin,

I've submitted "JDK-8013171 G1: C1 x64_64 barriers use 32 bit accesses 
to 64-bit PtrQueue::_index" to track the C1 barrier change. It should be 
visible in a day or so.

Cheers,

JohnC

On 4/24/2013 1:50 AM, Doerr, Martin wrote:
>
> Hi John,
>
> thanks for your input. Here's the new webrev:
>
> http://cr.openjdk.java.net/~goetz/webrevs/8012715/ 
> <http://cr.openjdk.java.net/%7Egoetz/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/f4bf22cf/attachment-0001.html