[m-rev.] for review: Conform to memory alignment requirements on doubles.

Julien Fischer jfischer at opturion.com
Fri Oct 11 17:23:18 AEDT 2013


Hi Peter,

On Mon, 7 Oct 2013, Peter Wang wrote:

> If desired, I could try doing the nondet stack change mentioned below.

For the 13.05 branch just boxing floats on the nondet stack will be
fine.  For the master branch, I'll leave it up to you.

...

> ---
>
> Conform to memory alignment requirements on doubles.
>
> On some 32-bit architectures, we were violating memory alignment
> requirements for double-precision floats, in cell fields and on det and
> nondet stacks.  Bug #299.
>
> We now only take the address of a double field when it occurs on an
> aligned memory address, i.e. when it starts at an even word offset from
> the start of the cell (this assumption is incompatible with term-size
> profiling which adds a hidden word before the start of the cell).
>
> For the det stack, we can round up allocations to keep the stack pointer
> double-aligned, then allocate slots for doubles at even word offsets
> from the stack pointer.
>
> It would be trickier for the nondet stack.  Multiple frame types exist
> on the nondet stack, and the different frame types are identified by
> their sizes: 3-word and 4-word temporary frames, and 5/6+ word ordinary
> frames. Rather than rounding up frame sizes to even numbers of words,
> we would probably want to dynamically pad ordinary frame allocations,
> such that any doubles in the frame will be at aligned addresses.
>
> However, in this change, we simply store box floats on the nondet stack.

...

> +double_width_floats_on_det_stack(Globals, FloatDwords) :-
> +    globals.lookup_int_option(Globals, bits_per_word, TargetWordBits),
> +    globals.lookup_bool_option(Globals, single_prec_float, SinglePrecFloat),
> +    ( TargetWordBits = 64 ->
> +        FloatDwords = no
> +    ; TargetWordBits = 32 ->
> +        (
> +            SinglePrecFloat = yes,
> +            FloatDwords = no
> +        ;
> +            SinglePrecFloat = no,
> +            FloatDwords = yes
> +        )
> +    ;
> +        unexpected($module, $pred, "bits_per_word not 32 or 64")
> +    ).

Aside: why do we do "unexpected($module, $pred, ...", instead of
"unexpected($file, $pred, ..."?  Since the module name is already
contained in $pred", there's no pointing printing out $module and since
file names can differ from module names using $file is actually more 
useful information.

...

The diff looks ok (modulo the comments from other reviewers).

Cheers,
Julien



More information about the reviews mailing list