[m-rev.] for review: Implement modechecking of coerce more efficiently.

[Zoltan Somogyi]

On Wed, 5 May 2021 16:41:28 +1000, Peter Wang <novalazy at gmail.com> wrote:
> > > +            HOInstInfoX = higher_order(_PredInstInfoX),
> > > +            UniqY = make_result_uniq(LiveX, UniqX),
> > > +            % Coerce cannot change the calling convention.
> > > +            InstY = ground(UniqY, HOInstInfoX),
> > > +            Res = yes(InstY)
> > > +        )

> However, I could well have missed something. If you have an example
> in mind then please post it.

No example, I just looked at the code that did not go into HOInstInfoX.

> > > +:- pred modecheck_coerce_from_ground_make_inst(module_info::in, tvarset::in,
> > > +    is_live::in, uniqueness::in, mer_type::in, mer_type::in,
> > > +    maybe(mer_inst)::out) is det.
> > > +
> > > +modecheck_coerce_from_ground_make_inst(ModuleInfo, TVarSet, LiveX, UniqX,
> > > +        TypeX, TypeY, MaybeInstY) :-
> > > +    ( if TypeX = TypeY then
> > > +        % This should be a common case.
> > > +        UniqY = make_result_uniq(LiveX, UniqX),
> > > +        InstY = ground(UniqY, none_or_default_func),
> > > +        MaybeInstY = yes(InstY)
> > > +    else if check_is_subtype(ModuleInfo, TVarSet, TypeX, TypeY) then
> > > +        set.init(SeenTypes0),
> > > +        modecheck_coerce_from_ground_make_inst_2(ModuleInfo, TVarSet,
> > > +            LiveX, UniqX, SeenTypes0, TypeX, TypeY, InstY),
> > > +        MaybeInstY = yes(InstY)
> > > +    else
> > > +        MaybeInstY = no
> > > +    ).
> > 
> > If I am not mistaken, this is called from exactly one place. I would inline it there,
> > and remove the _2 from the name of the next predicate.
> > 
> 
> It's called recursively as well.

There is no direct recursion in the code above. When I do a search for
modecheck_coerce_from_ground_make_inst in your message, I get the
declaration and the definition above, the call from modecheck_coerce_make_inst,
and all the other matches have a _2 at the end.
 
> > All the error messages here give just one side of the story.
> > They give the actual inst, but not the expected inst, or better yet,
> > the diff between those two insts.
> 
> Right. The hard part may be how to report a problem somewhere deep
> in an inst tree.

Hence my suggestion in my previous email: report each problem as
something like "in arg a1 of function symbol f1, which is inside
arg a2 of f2, which is inside arg a3 of f3, inside var V, expected this,
got that". One could argue whether the description of the position
should go from problem side to the top var or vice versa, but
that is the info I would want if I got this error message.

> > The diff is otherwise fine. I would be surprised if it had the bad performance
> > of the earlier attempt, but did you measure its impact?
> 
> The occurrences I have take no time at all.

Excellent!

Zoltan.