flame / blis

BLAS-like Library Instantiation Software Framework
Other
2.18k stars 362 forks source link

Default BLIS_[MNK]T values never actually set #781

Closed devinamatthews closed 8 months ago

devinamatthews commented 8 months ago

In bli_cntx_init_ref.c, the default values for the gemmsup thresholds (BLIS_[MNK]T blocksizes) are set to zero, so that no operation ever matches the criteria for gemmsup. HOWEVER, these are set via bli_cntx_set_blkszs which calls bli_cntx_set_blksz which calls bli_blksz_copy_if_pos. Thus, the zero values are simply discarded and those blocksizes remain uninitialized. The upshot of this is that the reference gemmsup handler is called for gemm problems essentially at random (and as it turns out, very rarely the reference gemmsup implementation encounters a divide-by-zero error). Because bli_gemmt?sup checks if m, n, and k are less than the thresholds, setting the default values to 1 should be safe and effective.

fgvanzee commented 8 months ago

HOWEVER, these are set via bli_cntx_set_blkszs which calls bli_cntx_set_blksz which calls bli_blksz_copy_if_pos.

I'm not seeing where bli_cntx_set_blkszs() calls bli_cntx_set_blksz(), but your overall point still seems to hold.

Am I missing something?

devinamatthews commented 8 months ago

The middle step is just in my branch but the end is the same.

fgvanzee commented 8 months ago

@devinamatthews Do you agree that this fixes the issue?

    ...

    if ( v_s >= 0 ) bli_blksz_set_def( v_s, BLIS_FLOAT,    b_dst );
    if ( v_d >= 0 ) bli_blksz_set_def( v_d, BLIS_DOUBLE,   b_dst );
    if ( v_c >= 0 ) bli_blksz_set_def( v_c, BLIS_SCOMPLEX, b_dst );
    if ( v_z >= 0 ) bli_blksz_set_def( v_z, BLIS_DCOMPLEX, b_dst );

    if ( e_s >= 0 ) bli_blksz_set_max( e_s, BLIS_FLOAT,    b_dst );
    if ( e_d >= 0 ) bli_blksz_set_max( e_d, BLIS_DOUBLE,   b_dst );
    if ( e_c >= 0 ) bli_blksz_set_max( e_c, BLIS_SCOMPLEX, b_dst );
    if ( e_z >= 0 ) bli_blksz_set_max( e_z, BLIS_DCOMPLEX, b_dst );
}
devinamatthews commented 8 months ago

Yes, or setting the default blocksizes to 1.

devinamatthews commented 8 months ago

OR, zeroing out all blocksizes and kernel pointers initially.

fgvanzee commented 8 months ago

Wait, now I'm doubting myself that the above code snippet is even a fix. 🤔

fgvanzee commented 8 months ago

Because bli_gemmt?sup checks if m, n, and k are less than the thresholds, setting the default values to 1 should be safe and effective.

Hmm, my concern is that with default thresholds of 1, problem sizes of 0 would cause the sup handler to be invoked, which I would prefer not to happen.

devinamatthews commented 8 months ago

In #710 the zero-dim cases are handled earlier.

fgvanzee commented 8 months ago

@devinamatthews Do you agree that this fixes the issue?

    ...

    if ( v_s >= 0 ) bli_blksz_set_def( v_s, BLIS_FLOAT,    b_dst );
    if ( v_d >= 0 ) bli_blksz_set_def( v_d, BLIS_DOUBLE,   b_dst );
    if ( v_c >= 0 ) bli_blksz_set_def( v_c, BLIS_SCOMPLEX, b_dst );
    if ( v_z >= 0 ) bli_blksz_set_def( v_z, BLIS_DCOMPLEX, b_dst );

    if ( e_s >= 0 ) bli_blksz_set_max( e_s, BLIS_FLOAT,    b_dst );
    if ( e_d >= 0 ) bli_blksz_set_max( e_d, BLIS_DOUBLE,   b_dst );
    if ( e_c >= 0 ) bli_blksz_set_max( e_c, BLIS_SCOMPLEX, b_dst );
    if ( e_z >= 0 ) bli_blksz_set_max( e_z, BLIS_DCOMPLEX, b_dst );
}

Okay, after studying the code a bit more, I'm back on board with this fix.