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

[Igor Veresov]

But that works correctly though. x86 is little endian. 

igor

On Apr 23, 2013, at 4:02 PM, John Cuthbertson <john.cuthbertson at oracle.com> wrote:

> Hi Martin,
> 
> One more thing - do you guys test with C1/Tiered at all?
> 
> I decided to check the C1 barriers and it looks like there's an issue there as well.
> 
> On Sparc we're doing the right thing. On x86 (in the G1 post barrier), I see:
> 
>>         // storing region crossing non-NULL, card is clean.
>>         // dirty card and log.
>> 
>>         __ movb(Address(card_addr, 0), 0);
>> 
>>         __ cmpl(queue_index, 0);
>>         __ jcc(Assembler::equal, runtime);
>>         __ subl(queue_index, wordSize);
> 
> And if we compare the same check for the pre-barrier:
> 
>>         // Can we store original value in the thread's buffer?
>> 
>> #ifdef _LP64
>>         __ movslq(tmp, queue_index);
>>         __ cmpq(tmp, 0);
>> #else
>>         __ cmpl(queue_index, 0);
>> #endif
>>         __ jcc(Assembler::equal, runtime);
>> #ifdef _LP64
>>         __ subq(tmp, wordSize);
>>         __ movl(queue_index, tmp);
>>         __ addq(tmp, buffer);
>> #else
>>         __ subl(queue_index, wordSize);
>>         __ movl(tmp, buffer);
>>         __ addl(tmp, queue_index);
>> #endif
> 
> I think both are wrong. In the pre-barrier we do a 32 bit move of the new index into the queue_index address. In the post barrier we use only 32 bit arithmetic. As you said they're benign as long as the values fit in 32 bits but they should probably be fixed.
> 
> Do you want to fix the C1 barrier as part of your webrev or have me fix it in a separate CR?
> 
> Thanks,
> 
> JohnC
> 
> On 4/23/2013 10:31 AM, John Cuthbertson wrote:
>> 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/
>>>  
>>> 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/021c5959/attachment.html