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

[Zoltan Somogyi]

On Fri, 16 Feb 2018 20:32:18 +1100 (AEDT), "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 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.

I am now pretty sure that the above is wrong :-)

The malformed constant structure is in fact generated by
the following chain of calls implementing As = [-128i8]:

generate_construction_2
construct_cell
assign_cell_to_var
var_locn_assign_cell_to_var
cell_is_constant
rval_type_as_arg
natural_type

The problem is that the last two in that chain were designed
to decide only the TYPE of an rval in a cell. They were not designed
to decide the SIZE of that rval, because when they were written,
every rval was exactly the same size: the size of MR_Word.

This assumption pervades pretty much the whole of global_data.m.
While some of the predicates in that module take an argument whose
type is arg_width (whose function symbols are full_word, double_word,
partial_word_first and partial_word_shifted), the last two functors
do not appear in global_data.m at all, and double_word appears
only in two occurrences of the test "ArgWidth \= double_word"
in the natural_type predicate. I just ran a bootcheck in which
both those tests were replaced by expect(unify(ArgWidth, full_word), ...),
and the bootcheck passed, which means that ArgWidth was *always*
full_word in those cases.

What this means that the *proper* fix for this bug will require
a thorough review of the LLDS code generator's handling of argument
packing, which I *can't* do in a weekend. A quick fix that works
for this bug *only* is possible in that time, but I cannot guarantee
that it won't break something else.

The wrinkle here is that going through the LLDS code generator
making sure that it works for packing small enums only (the current
situation) would be wasteful, when in only a few days or a week,
*after* the time needed to find any problems by the new du_type_layout.m,
I would like to modify du_type_layout.m to pack not just enums
but also dummy values and {int,uint}{8,16,32}s inside words as well.
These introduce additional concerns that this "going through" should
watch out for.

For example, extracting a packed enum from a word always uses zero-fill,
but extracting e.g. a packed int8 requires sign-fill (sign extension).
And explicitly packing dummy args will avoid situations like the following.
Given the types

:- type enum1
    --->    e1a
    ;       e1b.

:- type enum2
    --->    e2a
    ;       e2b
    ;       e2c.

:- type dum
    --->    dum.

:- type x
    --->    x(enum1, enum2, dum, enum1, enum2).

the compiler will now lay out values of type x like this:

:- type c.x
  --->    x(c.enum1, c.enum2, c.dum, c.enum1, c.enum2).
          % tag: single_functor_tag
          % packed arg widths:
          % arg 1: partial_word_first at offset 0, mask 1
          % arg 2: partial_word_shifted at offset 0, shift 1, mask 3
          % arg 3: full_word at offset 1
          % arg 4: partial_word_first at offset 2, mask 1
          % arg 5: partial_word_shifted at offset 2, shift 1, mask 3

What should have been storable in one word is stored in three,
with the argument that *should* take least number of bits (0),
actually taking the most bits. 

What I am saying is that the right way to fix this is to wait
for any problems with the new du_type_layout.m to show up,
fix them if they occur, and *then* fix this bug, in a compiler
that will by then allow e.g. int8s to occur not just as the only
thing in a word but also as one of several arguments packed
together in a word. This will push things out to (probably)
thursday or friday.

Julien, would you be ok with that? If not, why not? If I know
your actual problem, maybe I can offer another solution.

Zoltan.