review (S) for 6982370: SIGBUS in jbyte_fill

[Vladimir Kozlov]

Tom Rodriguez wrote:
>> Could you use shifts?:
>>
>>      __ sllx(O3, 60, O3);
>>      __ srlx(O3, 60, O3);
> 
> Yep.  You mean 48 don't you?

Yes, sorry mmy mistake.

> 
>> instead of:
>>
>> 1641       __ set(0xffff, O3); << 2 instructions
>> 1642       __ and3(value, O3, value);
>>
>> Tom, sorry I don't like your changes which increase the code.
> 
> The normal path doesn't have to split the loads since it knows they are aligned so it seems like a waste to emit extra stores there just so it can be used in the case where it's very small.  I could consolidate the slow path more but I don't want to make the main path slower.

OK, I got it. Now I agree with your code. It is good to go.

Thanks,
Vladimir

> 
> tom
> 
>> How about this:
>>
>>    __ BIND(L_fill_4_bytes);
>>    __ brx(Assembler::zero, false, Assembler::pt, L_fill_2_bytes);
>>    if (t == T_BYTE || t == T_SHORT) {
>>      __ delayed()->andcc(count, 1<<(shift-1), G0);
>> +     if (t == T_SHORT) {
>> +       __ sth(value, to, 0);
>> +       __ sth(value, to, 2);
>> +     } else {
>> +       __ stb(value, to, 0);
>> +       __ stb(value, to, 1);
>> +       __ stb(value, to, 2);
>> +       __ stb(value, to, 3);
>> +     }
>>    } else {
>>      __ delayed()->nop();
>> +     __ stw(value, to, 0);
>>    }
>> -   __ stw(value, to, 0);
>>    if (t == T_BYTE || t == T_SHORT) {
>>      __ inc(to, 4);
>>      // fill trailing 2 bytes
>>      __ andcc(count, 1<<(shift-1), G0); // in delay slot of branches
>>      __ BIND(L_fill_2_bytes);
>>      __ brx(Assembler::zero, false, Assembler::pt, L_fill_byte);
>>      __ delayed()->andcc(count, 1, count);
>> -     __ sth(value, to, 0);
>>      if (t == T_BYTE) {
>> +       __ stb(value, to, 0);
>> +       __ stb(value, to, 1);
>>        __ inc(to, 2);
>>        // fill trailing byte
>>        __ andcc(count, 1, count);  // in delay slot of branches
>>        __ BIND(L_fill_byte);
>>        __ brx(Assembler::zero, false, Assembler::pt, L_exit);
>>        __ delayed()->nop();
>>        __ stb(value, to, 0);
>>      } else {
>> +       __ sth(value, to, 0);
>>        __ BIND(L_fill_byte);
>>      }
>>    } else {
>>      __ BIND(L_fill_2_bytes);
>>    }
>>    __ BIND(L_exit);
>>
>> Vladimir
>>
>> Tom Rodriguez wrote:
>>> http://cr.openjdk.java.net/~never/6982370
>>> 6982370: SIGBUS in jbyte_fill
>>> Summary:
>>> Reviewed-by:
>>> In the new fill routines copy less than 8 bytes long skip over the
>>> main code and go the final cleanup stage.  On sparc this isn't right
>>> since it still hasn't accounted for alignment.  The fix is to have a
>>> special path for handling that.  The code doesn't crash on 32 bit
>>> sparc because handling of misaligned stores seems to be enabled.
>>> Additionally I noticed a problem with the logic for clearing the high
>>> bits of a short value.  The logic I wrote works correctly for clean
>>> ints but doesn't work for 64 bit signed extended ints resulting in
>>> invalid fill values in some cases.  Tested with failing nightly and
>>> new test case to exercise the various alignments.
>