Possible refactoring for __stg_gc_fun in HeapStackCheck.cmm

19 Mar 2014

      Hi list!
This is my first mail to the list; please correct me if I made any mistakes.

I’ve been studying GHC source code for studying’s sake, and think I’ve found a few problems in this function.

Below comes the function in its current form:
————————————————————
__stg_gc_fun /* explicit stack */
{
    W_ size;
    W_ info;
    W_ type;

    info = %GET_FUN_INFO(UNTAG(R1));

    // cache the size
    type = TO_W_(StgFunInfoExtra_fun_type(info));
    if (type == ARG_GEN) {
	size = BITMAP_SIZE(StgFunInfoExtra_bitmap(info));
    } else { 
	if (type == ARG_GEN_BIG) {
#ifdef TABLES_NEXT_TO_CODE
            // bitmap field holds an offset
            size = StgLargeBitmap_size( StgFunInfoExtra_bitmap(info)
                                        + %GET_ENTRY(UNTAG(R1)) /* ### */ );
#else
	    size = StgLargeBitmap_size( StgFunInfoExtra_bitmap(info) );
#endif
	} else {
	    size = BITMAP_SIZE(W_[stg_arg_bitmaps + WDS(type)]);
	}
    }

#ifdef NO_ARG_REGS
    // we don't have to save any registers away
    Sp_adj(-3);
    Sp(2) = R1;
    Sp(1) = size;
    Sp(0) = stg_gc_fun_info;
    jump stg_gc_noregs [];
#else
    W_ type;
    type = TO_W_(StgFunInfoExtra_fun_type(info));
    // cache the size
    if (type == ARG_GEN || type == ARG_GEN_BIG) {
        // regs already saved by the heap check code
        Sp_adj(-3);
        Sp(2) = R1;
        Sp(1) = size;
        Sp(0) = stg_gc_fun_info;
        // DEBUG_ONLY(foreign "C" debugBelch("stg_fun_gc_gen(ARG_GEN)"););
        jump stg_gc_noregs [];
    } else { 
        jump W_[stg_stack_save_entries + WDS(type)] [*]; // all regs live
	    // jumps to stg_gc_noregs after saving stuff
    }
#endif /* !NO_ARG_REGS */
}

The problems I see:
1. “type” variable is declared and its value calculated twice,
2. if (type == ARG_GEN … check is needlessly done twice,
3. “size" value is calculated and then discarded in !NO_ARG_REGS, for types other than ARG_GEN or ARG_GEN_BIG, and
4. (this is a minor one) Sp_adj(-3) ~ jmup_stg_gc_noregs code is duplicated. I know they’re in separate #if branch but still...

And my proposed fixes are:
————————————————————
__stg_gc_fun /* explicit stack */
{
    W_ size;
    W_ info;
    W_ type;

    info = %GET_FUN_INFO(UNTAG(R1));

    // cache the size
    type = TO_W_(StgFunInfoExtra_fun_type(info));
    if (type == ARG_GEN) {
        // regs already saved by the heap check code
	size = BITMAP_SIZE(StgFunInfoExtra_bitmap(info));
    } else { 
	if (type == ARG_GEN_BIG) {
	    // regs already saved by the heap check code
#ifdef TABLES_NEXT_TO_CODE
            // bitmap field holds an offset
            size = StgLargeBitmap_size( StgFunInfoExtra_bitmap(info)
                                        + %GET_ENTRY(UNTAG(R1)) /* ### */ );
#else
	    size = StgLargeBitmap_size( StgFunInfoExtra_bitmap(info) );
#endif
	} else {
#ifdef NO_ARG_REGS
	    // we don't have to save any registers away
	    size = BITMAP_SIZE(W_[stg_arg_bitmaps + WDS(type)]);
#else
	    jump W_[stg_stack_save_entries + WDS(type)] [*]; // all regs live
	    // jumps to stg_gc_noregs after saving stuff
#endif
	}
    }

    Sp_adj(-3);
    Sp(2) = R1;
    Sp(1) = size;
    Sp(0) = stg_gc_fun_info;
    jump stg_gc_noregs [];
}

What do you guys think?

Thanks,
Horsng Junn