[m-rev.] problems with abs for int{8,16,32}

Julien Fischer jfischer at opturion.com
Fri Feb 16 20:56:29 AEDT 2018



On Fri, 16 Feb 2018, Zoltan Somogyi wrote:

> On Fri, 16 Feb 2018 03:47:20 -0500 (EST), Julien Fischer <jfischer at opturion.com> wrote:
>> If you have not fixed this or are not intending to fix it anytime soon,
>> could you please let me know and I will have a look at it this weekend.
>
> I just committed two diffs cleaning up some aspects of unify_gen.m,
> and I intended to commit a third before fixing the problem.
>
> I am pretty sure the bug lies in the fact that generate_const_struct
> ignores the cs_term_type field of ConstStruct. It should look up the
> representation of the type, and use that info in its task.
>
> The current code of generate_cons_struct_arg_tag handles each arg
> independently; this needs to be fixed to allow it to combine the values
> of 2+ args that happen to be packed into the same word.

I was wondering about that in light of all your recent changes.

> The code that does that can then set the "typed" part of the typed-rval
> it generates to a word-sized type (I expect lt_integer for such packed words).
> As it is, the current code of generate_cons_struct_arg_tag almost ignores ArgWidth;
> using the natural type of the int8 argument for the type part of the typed-rval
> is what causes the wrong type for the static structure generated for [-128i8].
>
> In ml_unify_gen.m, the code that constructs static terms is modelled
> after the code that constructs dynamic terms, with (IIRC) close to the
> minimum changes required. This should also be the case for unify_gen.m,
> but I wouldn't want to take the code generating dynamic terms as the model
> for the static case just yet. This is because generate_cons_args *also*
> ignores argument packing, and requires a post-pass (pack_cell_rvals)
> to fix this. I would prefer to replace the generate_cons_args/pack_cell_rvals
> two-pass structure with a single pass structure in the dynamic case
> before using it as the model for the static case. This is the third
> cleanup I mentioned above.
>
> I won't get to either the third cleanup or the bug fix before tomorrow morning
> at the earliest, and I intend to go shopping then, but I was expecting
> to finish both by the end of sunday. Is that soon enough for you,
> or do you want to take over from here?

That's soon enough for me; I have further changes related to fixed size
integers coming up so I just didn't want there to be too long a delay.

Julien.


More information about the reviews mailing list