Possible refactoring for __stg_gc_fun in HeapStackCheck.cmm

[Simon Marlow]

On 19/03/2014 03:06, Horsng Junn wrote:
> Hi list!
> This is my first mail to the list; please correct me if I made any mistakes.

Welcome :-)

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

Yes.

> 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

Yes, but this isn't a performance-critical bit of code, and a good 
backend will be able to optimise it anyway.  Clarity is more important.

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

I think your version has more complex control-flow: whether we drop out 
of the if expression to the code that follows depends on an #ifdef. 
I'd rather have something like this:

if (MAX_REAL_VANILLA_REG < 2 || type == ARG_GEN || type == ARG_GEN_BIG) {
   Sp_adj(-3);
   ...
} else {
   jump W_[stg_stack_save_entries + WDS(type)] [*];
}

That way the tail-calls are all at the end of the control flow.

If you validate a patch and attach it to a ticket, I'll take a look.

Cheers,
	Simon



> ————————————————————
> __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
>
> _______________________________________________
> ghc-devs mailing list
> ghc-devs at haskell.org
> http://www.haskell.org/mailman/listinfo/ghc-devs
>