[m-rev.] for review: Reduce memory requirement to build compiler in profdeep grade.

[Peter Wang]

On Wed, 12 May 2021 15:57:26 +1000 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> 2021-05-11 18:07 GMT+10:00 "Peter Wang" <novalazy at gmail.com>:
> > output_int_binop_for_java needs to handle various cases that require
> > extra casts/masking, so I don't think it's that simple. I left it as-is.
> 
> OK, so I have done it instead. The diff is attached, the first bootcheck
> is now on to the tests. For review by you, Peter, since you have the right
> details swapped in.
> 
> Note that the diff is hard to read, even with diff --patience, because it
> deletes almost twice as many lines as it adds. I think the best way to review it
> would be to just apply it and read the result, or to read only the lines
> starting with + in the diff. I think the new version of the code, besides
> being shorter, is also much more readable.

It does look a lot better.

> 
> Does anyone know why in some cases, when the result of a binop
> needs to be cast, we output
> 
>    (cast-to-type) (operand1 op operand2)
> 
> while in other cases we output
> 
>   ((cast-to-type) (operand1 op operand2))
> 
> In other words, why we put parentheses around the whole thing
> in some cases but not in others?

I wouldn't trust my knowledge of operator precedences (and even if I did),
I could see myself writing the latter just in case. Maybe whoever wrote
it was thinking the same thing.

> diff --git a/compiler/mlds_to_java_data.m b/compiler/mlds_to_java_data.m
> index 32d0fc710..820750da1 100644
> +++ b/compiler/mlds_to_java_data.m
> @@ -573,287 +576,242 @@ output_binop_for_java(Info, Stream, Op, X, Y, !IO) :-
>  
>  output_int_binop_for_java(Info, Stream, Op, X, Y, !IO) :-
>      (
> +        ( Op = int_add(Type),   OpStr = "+"
> +        ; Op = int_sub(Type),   OpStr = "-"
> +        ; Op = int_mul(Type),   OpStr = "*"
> +        ),
> +        (
> +            ( Type = int_type_int
> +            ; Type = int_type_int64
> +            ; Type = int_type_int32

Reorder those two.

> +            ; Type = int_type_uint
> +            ; Type = int_type_uint32
> +            ; Type = int_type_uint64
> +            ),
> +            output_basic_binop_maybe_with_enum_for_java(Info, Stream,
> +                OpStr, X, Y, !IO)
> +        ;
> +            ( Type = int_type_int8,     Cast = "(byte) "
> +            ; Type = int_type_int16,    Cast = "(short) "
> +            ; Type = int_type_uint8,    Cast = "(byte) "
> +            ; Type = int_type_uint16,   Cast = "(short) "
> +            ),
> +            io.write_string(Stream, Cast, !IO),
> +            % XXX Document why we aren't calling
> +            % output_basic_binop_maybe_with_enum_for_java here.
> +            output_basic_binop_for_java(Info, Stream,
> +                OpStr, X, Y, !IO)
> +        )
> +    ;
> +        ( Op = int_div(Type),   OpStr = "/"
> +        ; Op = int_mod(Type),   OpStr = "%"
> +        ),
> +        (
> +            ( Type = int_type_int
> +            ; Type = int_type_int32
> +            ; Type = int_type_int64
> +            ),
> +            output_basic_binop_maybe_with_enum_for_java(Info, Stream,
> +                OpStr, X, Y, !IO)
> +        ;
> +            ( Type = int_type_int8,     Cast = "(byte) "
> +            ; Type = int_type_int16,    Cast = "(short) "
> +            ),
> +            io.write_string(Stream, Cast, !IO),
> +            output_basic_binop_for_java(Info, Stream,
> +                OpStr, X, Y, !IO)
> +        ;
> +            ( Type = int_type_uint8,    Cast = "(byte)",    Mask = "0xff"
> +            ; Type = int_type_uint16,   Cast = "(short)",   Mask = "0xffff"
> +            ; Type = int_type_uint32,   Cast = "(int)",     Mask = "0xffffffff"
> +            ; Type = int_type_uint,     Cast = "(int)",     Mask = "0xffffffff"
> +            ),
> +            io.format(Stream, "(%s ", [s(Cast)], !IO),
> +            output_basic_binop_with_mask_for_java(Info, Stream,
> +                OpStr, Mask, X, Y, !IO),
> +            io.write_string(Stream, ")", !IO)
> +        ;
> +            Type = int_type_uint64,
> +            % We could compute FuncName along with OpStr above,
> +            % but uint64 operands are rare enough that it is better
> +            % not to burden the non-uint64 code path with recording FuncName.
> +            ( Op = int_div(_), FuncName = "java.lang.Long.divideUnsigned"
> +            ; Op = int_mod(_), FuncName = "java.lang.Long.remainderUnsigned"
> +            ),
> +            output_binop_func_call_for_java(Info, Stream,
> +                FuncName, X, Y, !IO)
> +        )
> +    ;
> +        ( Op = int_lt(Type),    OpStr = "<"
> +        ; Op = int_gt(Type),    OpStr = ">"
> +        ; Op = int_le(Type),    OpStr = "<="
> +        ; Op = int_ge(Type),    OpStr = ">="
> +        ),
> +        (
> +            ( Type = int_type_int
> +            ; Type = int_type_int8
> +            ; Type = int_type_int16
> +            ; Type = int_type_int32
> +            ; Type = int_type_int64
> +            ),
> +            output_basic_binop_maybe_with_enum_for_java(Info, Stream,
> +                OpStr, X, Y, !IO)
> +        ;
> +            ( Type = int_type_uint8,    Mask = "0xffL"
> +            ; Type = int_type_uint16,   Mask = "0xffffL"

Remove the 'L' suffixes. They weren't there before.

> +            ; Type = int_type_uint32,   Mask = "0xffffffffL"
> +            ; Type = int_type_uint,     Mask = "0xffffffffL"
> +            ),
> +            output_basic_binop_with_mask_for_java(Info, Stream,
> +                OpStr, Mask, X, Y, !IO)
> +        ;
> +            Type = int_type_uint64,
> +            io.write_string(Stream, "(", !IO),
> +            output_binop_func_call_for_java(Info, Stream,
> +                "java.lang.Long.compareUnsigned", X, Y, !IO),
> +            io.format(Stream, " %s 0)", [s(OpStr)], !IO)
> +        )
> +    ;
> +        ( Op = bitwise_and(Type),   OpStr = "&"
> +        ; Op = bitwise_or(Type),    OpStr = "|"
> +        ; Op = bitwise_xor(Type),   OpStr = "^"
> +        ),
> +        (
> +            ( Type = int_type_int
> +            ; Type = int_type_int32
> +            ; Type = int_type_int64
> +            ; Type = int_type_uint
> +            ; Type = int_type_uint32
> +            ; Type = int_type_uint64
> +            ),
> +            output_basic_binop_maybe_with_enum_for_java(Info, Stream,
> +                OpStr, X, Y, !IO)
> +        ;
> +            ( Type = int_type_int8,     Cast = "(byte) "
> +            ; Type = int_type_int16,    Cast = "(short) "
> +            ; Type = int_type_uint8,    Cast = "(byte) "
> +            ; Type = int_type_uint16,   Cast = "(short) "
> +            ),
> +            io.write_string(Stream, Cast, !IO),
> +            output_basic_binop_for_java(Info, Stream,
> +                OpStr, X, Y, !IO)
> +        )
> +    ;
> +        (
> +            Op = unchecked_left_shift(Type, _ShiftByType),
> +            OpStr = "<<"
> +        ;
> +            Op = unchecked_right_shift(Type, _ShiftByType),
> +            (
> +                ( Type = int_type_int
> +                ; Type = int_type_int8
> +                ; Type = int_type_int16
> +                ; Type = int_type_int32
> +                ; Type = int_type_int64
> +                ),
> +                OpStr = ">>"
> +            ;
> +                ( Type = int_type_uint
> +                ; Type = int_type_uint8
> +                ; Type = int_type_uint16
> +                ; Type = int_type_uint32
> +                ; Type = int_type_uint64
> +                ),
> +                OpStr = ">>>"
> +            )
> +        ),
> +        % We ignore the distinction between shift_by_int and
> +        % shift_by_uint, because when targeting Java, we represent
>          % Mercury uints as Java ints anyway.
> +        (
> +            ( Type = int_type_int
> +            ; Type = int_type_int32
> +            ; Type = int_type_int64
> +            ; Type = int_type_uint
> +            ; Type = int_type_uint32
> +            ; Type = int_type_uint64
> +            ),
> +            output_basic_binop_maybe_with_enum_for_java(Info, Stream,
> +                OpStr, X, Y, !IO)
> +        ;
> +            ( Type = int_type_int8,     Cast = "(byte) ",   Mask = ""
> +            ; Type = int_type_int16,    Cast = "(short) ",  Mask = ""
> +            ; Type = int_type_uint8,    Cast = "(byte) ",   Mask = "0xff"
> +            ; Type = int_type_uint16,   Cast = "(short) ",  Mask = "0xffff"
> +            ),
> +            io.write_string(Stream, Cast, !IO),
> +            ( if
> +                Op = unchecked_right_shift(_, _),
> +                Mask \= ""
> +            then
> +                output_basic_binop_with_mask_for_java(Info, Stream,
> +                    OpStr, Mask, X, Y, !IO)

output_basic_binop_with_mask_for_java will mask both X and Y,
whereas the old code only masked X.

A comment about what Mask does would prevent questions about why it
isn't needed for left shifts. It converts a signed byte or short value
into a positive int value, thus preventing shifting in 1s from the left
when right shifting.

> +            else
> +                output_basic_binop_for_java(Info, Stream,
> +                    OpStr, X, Y, !IO)
> +            )
> +        )
>      ).
>  

The rest looks fine.

Peter