[m-rev.] for review: disable_warnings scope

[Zoltan Somogyi]

On Mon, 9 Jan 2017 17:08:39 +1100, Paul Bone <paul at bone.id.au> wrote:

> On Mon, Jan 09, 2017 at 12:46:32AM +1100, Zoltan Somogyi wrote:
> > This is the scope several of us discussed on friday.
> > 
> > For review by Paul. I would particularly like to know why warnings
> > about recursive calls that are not TAIL recursive are generated in
> > two separate places, and whether there is any particular reason why
> > we can't delete the version in ml_tailcalls.m, and make mark_tail_calls.m
> > do the job in all grades. I also added some XXXs that I would like his
> > opinion on.
> 
> The LLDS and MLDS backends implement tailcalls differently.

In some obvious respects, yes. In several other respects,
they treat them the same way.

>  Once TCO has
> taken place, or at least tail calls are identified (LLDS), _then_ warnings
> are generated.

For all backends, the warnings are generated BEFORE the generation
of any code (MLDS or LLDS) that exhibits tail recursion. Tail recursion
cannot even be expressed in HLDS, except by adding the tailcall feature
to a call goal.

>  The warnings that need to be generated depends on which
> things became tail calls which is different depending on the backend (eg:
> MLDS doesn't support mutual recursion

You are right, the MLDS backend does not (yet) support tail recursion.
However, there are other things that also affect whether a recursive call
will end up being a *tail* recursive call; the "last call modulo constructor"
optimization is one such thing.

>  Also because
> these bakends perform TCO at different stages, the warnings are generated at
> different stages.

Currently, the warnings are generated at different stages, but I think there is
no real reason behind this fact; it is just historical happenstance.

> If I remember completely.
> 
> LLDS:
>     Tail calls are identified in an HLDS stage,
>     Warnings are generated in (the same?) HLDS stage.
>     TCO is performed during HLDS->LLDS conversion.

No, that is not quite right.

Traditionally, the LLDS code generator didn't know which calls are tail calls;
it was only the optimizer that turned a call whose return label was the
procedure epilogue into a tail call. That changed *very* slightly when
I implemented (a) marking calls as tail calls for the debugger, and (b)
suppressing tail call optimization for use in cases where the stack frame
may still be needed by parallel conjuncts.
 
> MLDS:
>     HLDS->MLDS conversion occurs.
>     Tail calls are identified _and_ optimised in one MLDS stage.
>     Warnings are generated in (the same?) MLDS stage.
>     
> If we wanted to unify these parts of the compiler we'd probably need to
> re-implement most of the MLDS tail call code.

What MLDS tail call code? And why?

I just checked, and

- when it finds a tailcall, ml_tailcall.m marks the MLDS call statement
   as a tail call for ml_optimize.m, but
- ml_optimize.m IGNORES this mark on the call statement, and does
   its OWN test on each call to see whether it is a self-tail-recursive call.

Given this fact, I am not sure whether ml_tailcall.m does anything useful
at all, which is in itself a bit worrying.

>  If we decide want to do
> that, it should probably be done at the same time as my MLDS mutual
> recursion work.

Would you mind filling me on that?

> Another option is to find whatever common code (should) exist between them
> and make that shared but otherwise keep them separate.  Then we will need to
> pass the ignore warnings scope information through to the MLDS backend.

I don't see any need for that.

The way things work for the LLDS backend at the moment is:

- if the user wants warnings for recursive calls that aren't tail recursive,
   we run mark_tail_calls.m to get the warnings;
- possibly we run some HLDS to HLDS transformations;
- we generate LLDS code; and
- while optimizing the LLDS code, we replace some calls with tail calls.

To make sure that step 1 generates the right warnings,
- it needs to know what calls the code generator and/or the optimizer
   will turn into tail calls, and
- it needs to be done late enough that none of the transformations
   done in step 2 change the code in ways that would invalidate
   that knowledge.

I believe both of those conditions hold. I see no reason why they wouldn't
hold for the MLDS backend as well. As you point out, the "knowledge
about the behavior of the optimizer" will depend on which backend's optimizer
gets invoked, but even here I see no difference beyond the treatment
of mutually recursive calls.

Note that if a HLDS transformation (such as last call modulo constructor)
can affect what calls end up being tail calls, we want to run them BEFORE
we generate the warnings ANYWAY, just the get warnings that reflect the
final code accurately.

> This second option is tempting because it keeps these separate things
> separate.  Different backends have very different needs, even though both
> need to do TCO.

What "needs" are you referring to?

>  This is my "gut feeling" preference, but I'm beginning to
> think that if we can handle both backends in mark_tail_calls.m (and generate
> warnings there) then actually do TCO for each backend separately that may be
> best.  Provided that we make the logic in mark_tail_calls very clear with
> respect to which backends support which types of TCO.  This is currently my
> favorite option.

That is what I was proposing from the start.

> > index 9f48d8e..a5b3e9f 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -114,6 +114,16 @@ Changes to the Mercury language:
> >    the compiler now generates an error if the goal inside the scope
> >    is not a switch on the named variable.
> >  
> > +* We have added a new kind of scope to the language for disabling warnings
> > +  within the scope. A goal such as
> > +
> > +      disable_warnings [singleton_vars] (
> > +        Goal
> > +      )
> > +
> > +  is equivalent to Goal, with the exception that the compiler will not generate
> > +  warnings about singleton variables inside Goal.
> > +
> >  * We have added an extension to include external files
> >    in pragma foreign_decl and pragma foreign_code declarations.
> 
> Regarding syntax/naming I would consider calling it "suppress_warnings".
> I don't think it's either better or worse, just something to consider.

I prefer disable warnings. What do other people think?
 
> > diff --git a/compiler/constraint.m b/compiler/constraint.m
> > index 15b8020..ec89dea 100644
> > --- a/compiler/constraint.m
> > +++ b/compiler/constraint.m
> > @@ -165,6 +165,17 @@ propagate_conj_sub_goal_2(Constraints, Goal0, FinalGoals, !Info) :-
> >      ;
> >          GoalExpr = scope(Reason, SubGoal0),
> >          (
> > +            Reason = disable_warnings(_, _),
> > +            % NOTE: This assumes that compiler passes that generate the
> > +            % warnings that could be disabled by this scope have all been run
> > +            % BEFORE program transformations such as constraint propagation.
> > +            % If they haven't been, then the transformations can hide warnings
> > +            % about code by moving them into these scopes, or can caused them
> > +            % to be generated when the user does not want them by moving
> > +            % the warned-about code out of such scopes.
> > +            propagate_goal(Constraints, SubGoal0, SubGoal, !Info),
> > +            FinalGoals = [hlds_goal(scope(Reason, SubGoal), GoalInfo)]
> > +        ;
> >              ( Reason = exist_quant(_)
> >              ; Reason = from_ground_term(_, from_ground_term_deconstruct)
> >              ; Reason = from_ground_term(_, from_ground_term_other)
> 
> That will probably be a problem.  Both backends process their tailcalls
> after this stage.

The comment is talking about which calls *could later* be turned into tail calls.
It is NOT supposed to be talking about calls which *have* been turned into tail calls,
since (as I explained about) that happens only in the backends' optimizers.
What gave you the impression that it did?

> > @@ -621,15 +633,18 @@ maybe_report_nontailcall(AtTail, Info, SymName, Arity, ProcId,
> >          PredInfo = Info ^ mtc_pred_info,
> >          WarnTailCalls = Info ^ mtc_warn_tail_calls,
> >          MaybeRequireTailRecursion = Info ^ mtc_maybe_require_tailrec,
> > +        % XXX I (zs) don't understand the reason why the code below
> > +        % looks like it does. It should be enough to have a SINGLE field in
> > +        % Info that says whether we should call report_nontail_recursive_call,
> > +        % and if so, with what severity.
> 
> At the time I didn't consider that it looked bad or anything.  Now that I
> look at it I can see that it is needed to support the tail recursion warning
> proposal we created about this time last year.  We decided that tail
> recursion warning pragmas should be able to say whether mutual recursion was
> permitted or self-recursion only.  Therefore whether a warning is given will
> depend upon the call site.  So MaybeRequireTailRecursion needs to tell us
> which callees we should create a warning for.  Here's the original code
> where you can see it.
> 
>         MaybeRequireTailRecursion = Info ^ mtc_maybe_require_tailrec,
>         (   
>             MaybeRequireTailRecursion = no,
>             (
>                 WarnTailCalls = warn_tail_calls,
>                 report_nontailcall(PredInfo, SymName, Arity, ProcId,
>                     Context, we_warning, Specs)
>             ;
>                 WarnTailCalls = do_not_warn_tail_calls,
>                 Specs = []
>             )       
>         ;
>             MaybeRequireTailRecursion = yes(RequireTailrecInfo),
>             (   
>                 RequireTailrecInfo = enable_tailrec_warnings(WarnOrError,
> HERE--->        	_Type, _),
>                 % TODO: Check recursion type to implement support for
>                 % mutual vs self recursion checking.
>                 report_nontailcall(PredInfo, SymName, Arity, ProcId,
>                     Context, WarnOrError, Specs)
>             ;   
>                 RequireTailrecInfo = suppress_tailrec_warnings(_),
>                 Specs = []
>             )
>         )   
> 
> That being said this switch could be simplified if we introduce a new type.  I
> don't mind.

I see no reason why the recursion-type cannot be handled in the one-field
design. I will switch this code to that, but I will do it in a separate diff.

> > +                % XXX Should we generate a warning if the name of a warning
> > +                % to disable is listed more than once?
> 
> It might be indicative of some other error if it shows that the developer
> meant to type one thing but typed another.  Other than that and poor style
> I can't imagine how this represents a problem in the program.

OK, I will add a warning, though not one that can be disabled by this scope :-)
 
> > +                list.sort_and_remove_dups(WarningsList0, WarningsList),
> > +                (
> > +                    WarningsList = [HeadWarning | TailWarnings],
> > +                    Goal = disable_warnings_expr(Context,
> > +                        HeadWarning, TailWarnings, SubGoal),
> > +                    MaybeGoal = ok1(Goal)
> > +                ;
> > +                    WarningsList = [],
> > +                    Pieces = cord.list(ContextPieces) ++
> > +                        [lower_case_next_if_not_first, words("Error:"),
> > +                        words("a"), fixed(Functor), words("scope"),
> > +                        words("should list at least one warning."), nl],
> 
> s/should/must

Done.

> > +The keyword starting this scope may be written
> > +either as @code{disable_warnings} or as @code{disable_warning}.
> > +This is intended to make the code read more naturally
> > +regardless of whether the list contains the name of
> > +more than one disabled warning category.
> > +
> 
> I don't think this is necessary but don't feel strongly about it.  I would
> be happy to have the plural one even if I disabled only a single warning.

We already have several keywords with two different spellings (e.g. initialize/
initialise). This fits in with that pattern, and it is no trouble to have it there anyway.

Zoltan.