[m-rev.] for preliminary review: bug 493 & 532 alterative fix

[Peter Wang]

On Fri, 14 May 2021 12:17:55 +1000 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> 2021-05-14 12:07 GMT+10:00 "Peter Wang" <novalazy at gmail.com>:
> > Maybe I can do a quick benchmark on the compiler as well,
> > but I doubt it will show any difference.
> 
> Running "size -A" on the .o files generated with each fix,
> and comparing the outputs would tell you about the impact
> on static allocation.

Good idea.

Browsing through the diffs in C files shows that an increase in .rodata
is not necessary better, and a decrease in .rodata not necessarily
worse, e.g.

    diff --git a/a/stage2/compiler/hlds.make_hlds.state_var.c b/b/stage2/compiler/hlds.make_hlds.state_var.c
    index 4ea6cc0..65c167e 100644
    --- a/a/stage2/compiler/hlds.make_hlds.state_var.c
    +++ b/b/stage2/compiler/hlds.make_hlds.state_var.c
    ...
    @@ -1263,12 +1263,6 @@ static /* final */ const MR_Box hlds__make_hlds__state_var_scalar_common_2[6][3]
	 ((MR_Box) ((MR_Unsigned) 0U)),
	 ((MR_Box) ((MR_Unsigned) 0U))
       },
    -  /* row 5 */
    -  {
    -    ((MR_Box) (((MR_Box) ((MR_Integer) 1)))),
    -    ((MR_Box) ((MR_Unsigned) 0U)),
    -    ((MR_Box) ((MR_Unsigned) 0U))
    -  },
     };

     static /* final */ const MR_Box hlds__make_hlds__state_var_scalar_common_3[1][4] = {
    @@ -2700,7 +2694,7 @@ hlds__make_hlds__state_var____Unify____svar_state_0_0(
     MR_Word MR_CALL
     hlds__make_hlds__state_var__new_svar_store_0_f_0(void)
     {
    -  return (MR_Word) (&hlds__make_hlds__state_var_scalar_common_2[5]);
    +  return (MR_Word) (&hlds__make_hlds__state_var_scalar_common_2[4]);
     }

     MR_Word MR_CALL

(row 4 and row 5 are identical)

So it's hard to interpret the numbers.

I did notice a problem with string.base_negative_accumulator,
which is written deliberately to try to avoid allocation:

    base_negative_accumulator(Base) = Pred :-
	% Avoid allocating a closure for the common bases.
	( if Base = 10 then
	    Pred = accumulate_negative_int(10)
	else if Base = 16 then
	    Pred = accumulate_negative_int(16)
	else if Base = 8 then
	    Pred = accumulate_negative_int(8)
	else if Base = 2 then
	    Pred = accumulate_negative_int(2)
	else
	    Pred = accumulate_negative_int(Base)
	).

With my change, the generated code would allocate the closures instead
of returning pointers to static data. I don't want to make any change
that would cause such a regression.

Unfortunately, due to the fix for bug 532, the LLDS backend now also
generates code that allocates the closures. Previously, by chance,
it would return pointers to static data because:
(1) --loop-invariants happens to run mark_static_terms first
(2) common.m happens to check for "construct_statically" constructions
    to avoid a problem on a different backend

Anyway, common.m is too eager to replace V = (integer) with V = Base.
Your const static db approach would work better.

Peter