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

[John Cuthbertson]

Hi Igor,

It does work correctly. That's why the other CR is a P4. ;) But it makes 
sense to clean it up though.

JohnC

On 4/24/2013 4:38 PM, Igor Veresov wrote:
> 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 <mailto: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/ 
>>>> <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/d4b1ba65/attachment-0001.html