Thanks for the reply, but I don’t think I’m ready to submit a patch yet. Hopefully someday :) Thanks, Horsng Junn On 2014-03-19, at 8:54 PM, Simon Marlow wrote:

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@haskell.org http://www.haskell.org/mailman/listinfo/ghc-devs