[m-rev.] (no subject)

[Julien Fischer]

Hi Zoltan,

On Fri, 1 May 2020, Zoltan Somogyi wrote:

> For review by anyone. I am especially seeking feedback on whether
> the changes documented in a comment as being needed for
> effectively inlining calls to checked division and shift operations
> are worth the trouble they cause. The main user-visible effect
> is that code that catches math domain errors would need to be
> modified, though in only a trivial manner.

...

> Report invalid divisions and shifts ...
> 
> ... and sketch how we could effectively inline checked divisions and shifts
> even without intermodule optimization, and how we could simplify shifts
> by unsigned amounts. These latter parts are not enabled yet.
> 
> Use state variable notation where warranted.

...

> diff --git a/compiler/simplify_goal_call.m b/compiler/simplify_goal_call.m
> index 51b6817e8..7677149af 100644
> --- a/compiler/simplify_goal_call.m
> +++ b/compiler/simplify_goal_call.m

...

> +            % XXX We could replace the checked shift with the code of the check
> +            % and the unchecked shift, but doing that would require getting
> +            % access to three things:
> +            %
> +            % 1. the predicate declaration for unsigned_lt, which currently
> +            %    is a private predicate in int.m, and
> +            %
> +            % 2. the definition of the wrapper we put around the text of
> +            %    the exception message, which currently is math.domain error.

By most measures domain_error/1 is already defined in the wrong module,
so moving it can be viewed as an improvement.  (It's defined in the math
module, which is intended to export various operations on floats, but
nevertheless used by all of the integer modules as well, even though
they don't need anything else in the math module.)

> +            %
> +            % 3. the definition of "throw" in exception.m
> +            %
> +            % Even if ModuleName is "int", we can't currently access the
> +            % first. Since math.m is never imported implicitly, we cannot
> +            % count on math.domain_error being available either. And while
> +            % exception.m *is* currently imported implicitly if the module
> +            % being compiled contains a try_expr or an atomic_expr, a module
> +            % that does not contain either may nevertheless contain a checked
> +            % shift operation.
> +            %
> +            % We should consider making unsigned_lt an exported predicate
> +            % in private_builtin.m,

That seems reasonable, it's never been documented and users who want such an
operation can easily cobble something together out of the publicly documented
support for unsigned integers.

>              % and math_domain_error a type in the
> +            % same module.

No. Users may want to catch such exceptions so the type needs be in a
publicly documented part of the library. My suggestions would be:

     1. in builtin.m

     2. in exception.m, alongside software_error/1

(Either way I would stick with the name domain_error/1 rather than
math_domain_error/1).

The diff itself looks fine.

Julien.