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

[John Cuthbertson]

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/20130423/0ee7b4bf/attachment.html