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
