review (S) for 6982370: SIGBUS in jbyte_fill

[Tom Rodriguez]

On Sep 3, 2010, at 1:11 PM, Vladimir Kozlov wrote:

> Tom,
> 
> Did you run your new test on x86 (32- and 64-bit)?

Yes I did.

> 
> I don't see that O4 and O5 are used after you save 'to', 'count'.

Sorry that was some debug code I left behind.

> 
> Could you use shifts?:
> 
>      __ sllx(O3, 60, O3);
>      __ srlx(O3, 60, O3);

Yep.  You mean 48 don't you?

> 
> 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.

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.