[m-users.] Too Slow

[Peter Wang]

Hi,

On Fri, 08 May 2015 13:12:46 +0100, matthias.guedemann at googlemail.com (Matthias Güdemann) wrote:
> Hi Peter,
> 
> thank you for your comments, I integrated them with the exception of
> two:
> 
> > Most foreign procs could be more succinctly written, e.g.
> 
> > :- pragma foreign_proc("C", mp_init(Value::in, Result::out,
> >                                     Mp_Int::out),
> >   [will_not_call_mercury, promise_pure, thread_safe],
> >   "
> >    Mp_Int = MR_GC_NEW_ATTRIB(mp_int, MR_ALLOC_ID);
> >    Result = mp_init(Mp_Int);
> >    if (Result == MP_OKAY) {
> >      Result = mp_set_int(Mp_Int, Value);
> >      }
> > ").
> 
> I fully agree with you, but I think the C compiler will do that
> anyway. As "--no-c-optimize" must be specified explicitly, I assume
> that at least some optimization will be done normally, and the
> superfluous ints will go away.

Yes.  I suggested it for readability.

> 
> ,----
> | to_int(A, N) :-
> |     ( ( A >= min_int, A =< max_int) ->
> |         mp_to_long(A, N)
> |     ;
> |         fail
> |     ).
> |
> | :- pred mp_to_long(mp_int::in, int::out)
> |     is det.
> | :- pragma foreign_proc("C",
> |                       mp_to_long(A::in, N::out),
> |                       [will_not_call_mercury, promise_pure, thread_safe],
> | "
> |   unsigned long n;
> |   n = mp_get_long(A);
> |   N = (int) n;
> | ").
> `----
> 
> > Hmm.
> 
> The predicate checks whether the value is between min and max int,
> shouldn't this therefore work? (I added the explicit cast to int at the
> end).

Mercury ints have C type `MR_Integer' which may be wider than C `int',
so the explicit cast to `int' could truncate the value.

On 64-bit Windows it may be the case that
sizeof(unsigned long) < sizeof(MR_Integer)

To avoid truncation, you might have to use the mp_*_long_long functions.
You can check long long is at least as wide as MR_Integer,
at compile-time, with:

:- pragma foreign_code("C", "
    MR_STATIC_ASSERT(mp_int, sizeof(unsigned long long) >= sizeof(MR_Integer));
").

mp_init/4 contains a mp_set_int call.

There is a problem with:

    mp_int(N) = Res :-
	mp_init(abs(N), Res0),
	( N < 0 ->
	    Res = -Res0
	;
	    Res = Res0
	).

abs(min_int) will overflow silently or crash the program.

> > It would be preferable to statically allocate the mp_ints for these
> > values.  Something like:
> 
> >     :- pragma foreign_decl("C", local, " static mp_int constant_one;
> > ...  ").
> 
> >     :- initialise init/2.
> 
> >     :- pred init(io::di, io::uo) is det.
> 
> >     init(!IO) :- ...
> 
> >     :- pragma foreign_proc("C", one = (X::out), [ ... ], " X =
> > &constant_one; ").
> 
> > But it can be left for later.
> 
> this is not done yet. Could this not simply be memoized?

Yes.  There's a bit of runtime overhead per call.

Peter