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 <[email protected]> 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 >> [email protected] >> http://www.haskell.org/mailman/listinfo/ghc-devs
_______________________________________________ ghc-devs mailing list [email protected] http://www.haskell.org/mailman/listinfo/ghc-devs
