Possible refactoring for __stg_gc_fun in HeapStackCheck.cmm

[Horsng Junn]

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 <marlowsd at gmail.com> 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 at haskell.org
>> http://www.haskell.org/mailman/listinfo/ghc-devs

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.haskell.org/pipermail/ghc-devs/attachments/20140320/1a4a195c/attachment-0001.html>