[m-rev.] for review: fix bug with polymorphic modes and intermodule optimization

Julien Fischer juliensf at cs.mu.OZ.AU
Mon Nov 7 19:44:36 AEDT 2005


On Mon, 7 Nov 2005, Mark Brown wrote:

> On 07-Nov-2005, Julien Fischer <juliensf at cs.mu.OZ.AU> wrote:
> > Estimated hours taken: 16
> > Branches: main
> >
> > Fix the problem that has been causing hard_coded/intermod_poly_mode to fail.
> >
> > When writing foreign_procs to .opt files, make sure that any inst variables
> > have the correct name.
>
> On a related note, the reference manual doesn't appear to say anything about
> the scope of inst variables.  Does the scope extend to foreign_proc decls
> (like how the scope of type variables extends to clauses), or can we use
> different names in the foreign_proc decl?  If the former is the case, then
> you should also check that the names in the foreign_proc match those in the
> selected mode.
>

I would argue that the scope of inst variables should extend to cover
foreign_procs, although the compiler currently doesn't enforce this
(and I'm not planning on fixing it as part of this change).

> >
> > compiler/add_pragma.m:
> > 	When searching for the predmode decl corresponding to a foreign_proc
> > 	allow for a renaming between inst variables.  If we don't do this,
> > 	then the compiler cannot find the corresponding predmode declaration
> > 	for opt_imported foreign_procs that have polymorphically moded
> > 	arguments.  (The reason this is occurring is a bit obscure, it it
>
> s/it it/it/
>
Fixed.

> > 	looks like the inst_vars in the predmode decl and those in the
> > 	foreign_proc are being allocated from different inst_varsets.  Tracing
> > 	this through in the debugger indicates that this happening quite early
> > 	on, it may be that we've never made the necessary connection between
> > 	the sets of inst variables and we've never used polymorphically moded
> > 	procedures extensively enough to notice.)
>
> See my comment below, in mercury_to_mercury.m.
>
> > Index: compiler/add_pragma.m
> > ===================================================================
> > RCS file: /home/mercury1/repository/mercury/compiler/add_pragma.m,v
> > retrieving revision 1.16
> > diff -u -r1.16 add_pragma.m
> > --- compiler/add_pragma.m	4 Nov 2005 03:40:42 -0000	1.16
> > +++ compiler/add_pragma.m	7 Nov 2005 04:03:27 -0000
> > @@ -1372,8 +1372,14 @@
> >              map__to_assoc_list(Procs, ExistingProcs),
> >              pragma_get_modes(PVars, Modes),
> >              (
> > -                get_procedure_matching_argmodes(ExistingProcs, Modes,
> > -                    !.ModuleInfo, ProcId)
> > +                % XXX We need to allow for a renaming between inst variables
> > +                % here, otherwise we won't find a match for opt_imported
> > +                % foreign_procs that have polymorphically moded arguments.
> > +                % This occurs because the inst vars in the foreign_proc
> > +                % and the corresponding mode declaration seem to be allocated
> > +                % from different inst_varsets.
> > +                get_procedure_matching_argmodes_with_renaming(ExistingProcs,
> > +                    Modes, !.ModuleInfo, ProcId)
>
> Why is this an XXX?  The variables come from different varsets, so it is only
> natural that we should have to bind the variables in one context to those in
> the other.
>

At the time I wrote that I wasn't sure what was going on.  It's
now not an XXX.

> > Index: compiler/mercury_to_mercury.m
> > ===================================================================
> > RCS file: /home/mercury1/repository/mercury/compiler/mercury_to_mercury.m,v
> > retrieving revision 1.272
> > diff -u -r1.272 mercury_to_mercury.m
> > --- compiler/mercury_to_mercury.m	28 Oct 2005 02:10:18 -0000	1.272
> > +++ compiler/mercury_to_mercury.m	7 Nov 2005 04:02:15 -0000
> > @@ -558,8 +560,10 @@
> >      ;
> >          Pragma = foreign_proc(Attributes, Pred, PredOrFunc, Vars, VarSet,
> >              PragmaCode),
> > +        % The inst_varset isn't available to us here.
> > +        InstVarset = varset.init,
>
> The varset in the foreign_proc pragma_type, although it is a prog_varset,
> appears to contain the names of the inst variables.  When parsing the
> foreign_proc declaration we just cast the varset to a prog_varset and that's
> it.  Instead, we should handle the varset similarly to the processing of
> pred_mode declarations, which also contain two sorts of variables (in this
> case, type variables and inst variables).  This will require an extra field
> to be added to the foreign_proc functor.

Done.

> Once you have made that change it should be fine to commit this, but you
> should still post the relative diff.
>

I'll commit it after bootchecking it again.  See below for the relative
diff.

Cheers,
Julien.

diff -u compiler/add_pragma.m compiler/add_pragma.m
--- compiler/add_pragma.m	7 Nov 2005 04:03:27 -0000
+++ compiler/add_pragma.m	7 Nov 2005 08:34:28 -0000
@@ -70,9 +70,9 @@

 :- pred module_add_pragma_foreign_proc(pragma_foreign_proc_attributes::in,
     sym_name::in, pred_or_func::in, list(pragma_var)::in, prog_varset::in,
-    pragma_foreign_code_impl::in, import_status::in, prog_context::in,
-    module_info::in, module_info::out, qual_info::in, qual_info::out,
-    io::di, io::uo) is det.
+    inst_varset::in, pragma_foreign_code_impl::in, import_status::in,
+    prog_context::in, module_info::in, module_info::out,
+    qual_info::in, qual_info::out, io::di, io::uo) is det.

 :- pred module_add_pragma_tabled(eval_method::in, sym_name::in, int::in,
     maybe(pred_or_func)::in, maybe(list(mer_mode))::in, import_status::in,
@@ -188,7 +188,7 @@
         module_add_foreign_import_module(Lang, Import, Context, !ModuleInfo)
     ;
         % Handle pragma foreign procs later on (when we process clauses).
-        Pragma = foreign_proc(_, _, _, _, _, _)
+        Pragma = foreign_proc(_, _, _, _, _, _, _)
     ;
         % Handle pragma tabled decls later on (when we process clauses).
         Pragma = tabled(_, _, _, _, _)
@@ -1253,7 +1253,8 @@
 %-----------------------------------------------------------------------------%

 module_add_pragma_foreign_proc(Attributes0, PredName, PredOrFunc, PVars,
-        VarSet, PragmaImpl, Status, Context, !ModuleInfo, !QualInfo, !IO) :-
+        ProgVarSet, _InstVarset, PragmaImpl, Status, Context, !ModuleInfo,
+        !QualInfo, !IO) :-
     %
     % Begin by replacing any maybe_thread_safe foreign_proc attributes
     % with the actual thread safety attributes which we get from the
@@ -1372,12 +1373,16 @@
             map__to_assoc_list(Procs, ExistingProcs),
             pragma_get_modes(PVars, Modes),
             (
-                % XXX We need to allow for a renaming between inst variables
-                % here, otherwise we won't find a match for opt_imported
-                % foreign_procs that have polymorphically moded arguments.
-                % This occurs because the inst vars in the foreign_proc
-                % and the corresponding mode declaration seem to be allocated
-                % from different inst_varsets.
+                % The inst variables for the foreign_proc declaration
+                % and predmode declarations are from different varsets.
+                % We cannot unify the argument modes directly because
+                % the representation of the inst variables may be different.
+                % Instead we need to allow for a renaming between the
+                % inst variables in the argument modes of the foreign_proc
+                % and those of the predmode declaration.
+                %
+                % XXX We should probably also check that each pair in
+                % the renaming has the same name.
                 get_procedure_matching_argmodes_with_renaming(ExistingProcs,
                     Modes, !.ModuleInfo, ProcId)
             ->
@@ -1385,7 +1390,7 @@
                 pred_info_arg_types(!.PredInfo, ArgTypes),
                 pred_info_get_purity(!.PredInfo, Purity),
                 clauses_info_add_pragma_foreign_proc(Purity, Attributes,
-                    PredId, ProcId, VarSet, PVars, ArgTypes, PragmaImpl,
+                    PredId, ProcId, ProgVarSet, PVars, ArgTypes, PragmaImpl,
                     Context, PredOrFunc, PredName, Arity, Clauses0, Clauses,
                     !ModuleInfo, !IO),
                 pred_info_set_clauses_info(Clauses, !PredInfo),
@@ -1838,10 +1843,11 @@
         PredOrFunc, Arity, ArgTypes, Status, Context, !ModuleInfo, !QualInfo,
         !IO) :-
     map__lookup(ProcTable, ProcID, ProcInfo),
-    varset__init(VarSet0),
-    varset__new_vars(VarSet0, Arity, Vars, VarSet),
+    varset__init(ProgVarSet0),
+    varset__new_vars(ProgVarSet0, Arity, Vars, ProgVarSet),
     proc_info_argmodes(ProcInfo, Modes),
-    fact_table_pragma_vars(Vars, Modes, VarSet, PragmaVars),
+    proc_info_inst_varset(ProcInfo, InstVarSet),
+    fact_table_pragma_vars(Vars, Modes, ProgVarSet, PragmaVars),
     fact_table_generate_c_code(SymName, PragmaVars, ProcID, PrimaryProcID,
         ProcInfo, ArgTypes, !.ModuleInfo, C_ProcCode, C_ExtraCode, !IO),

@@ -1852,7 +1858,7 @@
     % Fact tables procedures should be considered pure.
     set_purity(purity_pure, Attrs2, Attrs),
     module_add_pragma_foreign_proc(Attrs, SymName, PredOrFunc, PragmaVars,
-        VarSet, ordinary(C_ProcCode, no), Status, Context,
+        ProgVarSet, InstVarSet, ordinary(C_ProcCode, no), Status, Context,
         !ModuleInfo, !QualInfo, !IO),
     ( C_ExtraCode = "" ->
         true
diff -u compiler/mercury_to_mercury.m compiler/mercury_to_mercury.m
--- compiler/mercury_to_mercury.m	7 Nov 2005 06:04:08 -0000
+++ compiler/mercury_to_mercury.m	7 Nov 2005 07:41:46 -0000
@@ -558,10 +558,10 @@
         Pragma = foreign_code(Lang, Code),
         mercury_output_pragma_foreign_body_code(Lang, Code, !IO)
     ;
-        Pragma = foreign_proc(Attributes, Pred, PredOrFunc, Vars, VarSet,
-            PragmaCode),
+        Pragma = foreign_proc(Attributes, Pred, PredOrFunc, Vars, ProgVarset,
+            InstVarset, PragmaCode),
         mercury_output_pragma_foreign_code(Attributes, Pred,
-            PredOrFunc, Vars, VarSet, varset.coerce(VarSet), PragmaCode, !IO)
+            PredOrFunc, Vars, ProgVarset, InstVarset, PragmaCode, !IO)
     ;
         Pragma = import(Pred, PredOrFunc, ModeList, Attributes, C_Function),
         mercury_format_pragma_import(Pred, PredOrFunc, ModeList,
only in patch2:
unchanged:
--- compiler/add_solver.m	28 Oct 2005 02:09:57 -0000	1.6
+++ compiler/add_solver.m	7 Nov 2005 07:55:46 -0000
@@ -183,10 +183,12 @@
     OutAnyMode        = out_mode(AnyInst),
     OutGroundMode     = out_mode(GroundInst),

-    VarSet0           = varset__init,
-    varset__new_var(VarSet0, X, VarSet1),
-    varset__new_var(VarSet1, Y, VarSet),
-
+    ProgVarSet0           = varset__init,
+    varset__new_var(ProgVarSet0, X, ProgVarSet1),
+    varset__new_var(ProgVarSet1, Y, ProgVarSet),
+
+    InstVarSet = varset__init,
+
     Attrs0            = default_attributes(c),
     some [!Attrs] (
         !:Attrs = Attrs0,
@@ -209,7 +211,8 @@
             ToGroundRepnSymName,
             function,
             ToGroundRepnArgs,
-            VarSet,
+            ProgVarSet,
+            InstVarSet,
             Impl
         ),
     ToGroundRepnItem = pragma(compiler(solver_type), ToGroundRepnForeignProc),
@@ -227,7 +230,8 @@
             ToAnyRepnSymName,
             function,
             ToAnyRepnArgs,
-            VarSet,
+            ProgVarSet,
+            InstVarSet,
             Impl
         ),
     ToAnyRepnItem = pragma(compiler(solver_type), ToAnyRepnForeignProc),
@@ -245,7 +249,8 @@
             FromGroundRepnSymName,
             function,
             FromGroundRepnArgs,
-            VarSet,
+            ProgVarSet,
+            InstVarSet,
             Impl
         ),
     FromGroundRepnItem = pragma(compiler(solver_type),
@@ -264,7 +269,8 @@
             FromAnyRepnSymName,
             function,
             FromAnyRepnArgs,
-            VarSet,
+            ProgVarSet,
+            InstVarSet,
             Impl
         ),
     FromAnyRepnItem = pragma(compiler(solver_type), FromAnyRepnForeignProc),
only in patch2:
unchanged:
--- compiler/make_hlds_passes.m	4 Nov 2005 03:40:49 -0000	1.20
+++ compiler/make_hlds_passes.m	7 Nov 2005 08:12:14 -0000
@@ -819,10 +819,10 @@
     Item = pragma(Origin, Pragma),
     (
         Pragma = foreign_proc(Attributes, Pred, PredOrFunc,
-            Vars, VarSet, PragmaImpl)
+            Vars, ProgVarSet, InstVarSet, PragmaImpl)
     ->
         module_add_pragma_foreign_proc(Attributes, Pred, PredOrFunc,
-            Vars, VarSet, PragmaImpl, !.Status, Context,
+            Vars, ProgVarSet, InstVarSet, PragmaImpl, !.Status, Context,
             !ModuleInfo, !QualInfo, !IO)
     ;
         Pragma = import(Name, PredOrFunc, Modes, Attributes, C_Function)
@@ -1124,7 +1124,8 @@
     Item = mutable(Name, _Type, InitTerm, Inst, MutAttrs),
     ( status_defined_in_this_module(!.Status, yes) ->
         module_info_get_name(!.ModuleInfo, ModuleName),
-        varset__new_named_var(varset.init, "X", X, VarSet0),
+        varset.new_named_var(varset.init, "X", X, ProgVarSet0),
+        InstVarset = varset.init,
         Attrs0 = default_attributes(c),
         set_may_call_mercury(will_not_call_mercury, Attrs0, Attrs1),
         ( mutable_var_thread_safe(MutAttrs) = thread_safe ->
@@ -1159,7 +1160,7 @@
         NonPureGetClause = pragma(compiler(mutable_decl),
             foreign_proc(GetAttrs,
                 mutable_get_pred_sym_name(ModuleName, Name), predicate,
-                [pragma_var(X, "X", out_mode(Inst))], VarSet0,
+                [pragma_var(X, "X", out_mode(Inst))], ProgVarSet0, InstVarset,
                 ordinary("X = " ++ TargetMutableName ++ ";", yes(Context)))),
         add_item_clause(NonPureGetClause, !Status, Context, !ModuleInfo,
             !QualInfo, !IO),
@@ -1192,7 +1193,7 @@
         ),
         NonPureSetClause = pragma(compiler(mutable_decl), foreign_proc(Attrs,
             mutable_set_pred_sym_name(ModuleName, Name), predicate,
-            [pragma_var(X, "X", in_mode(Inst))], VarSet0,
+            [pragma_var(X, "X", in_mode(Inst))], ProgVarSet0, InstVarset,
             ordinary(TrailCode ++ TargetMutableName ++ " = X;",
                 yes(Context)))),
         add_item_clause(NonPureSetClause, !Status, Context, !ModuleInfo,
@@ -1208,8 +1209,8 @@
         ( mutable_var_attach_to_io_state(MutAttrs) = yes ->
             set_tabled_for_io(tabled_for_io, Attrs0, PureIntAttrs0),
             set_purity(purity_pure, PureIntAttrs0, PureIntAttrs),
-            varset.new_named_var(VarSet0, "IO0", IO0, VarSet1),
-            varset.new_named_var(VarSet1, "IO",  IO,  VarSet),
+            varset.new_named_var(ProgVarSet0, "IO0", IO0, ProgVarSet1),
+            varset.new_named_var(ProgVarSet1, "IO",  IO,  ProgVarSet),
             PureSetClause = pragma(compiler(mutable_decl),
                 foreign_proc(PureIntAttrs,
                     mutable_set_pred_sym_name(ModuleName, Name), predicate,
@@ -1217,7 +1218,7 @@
                         pragma_var(X,   "X",   in_mode(Inst)),
                         pragma_var(IO0, "IO0", di_mode),
                         pragma_var(IO,  "IO",  uo_mode)
-                    ], VarSet,
+                    ], ProgVarSet, InstVarset,
                     ordinary(TargetMutableName ++ " = X; IO = IO0;",
                         yes(Context)
                     )
@@ -1232,7 +1233,7 @@
                         pragma_var(X,   "X",   out_mode(Inst)),
                         pragma_var(IO0, "IO0", di_mode),
                         pragma_var(IO,  "IO",  uo_mode)
-                    ], VarSet,
+                    ], ProgVarSet, InstVarset,
                     ordinary(
                                 "X = " ++ TargetMutableName ++ ";" ++
                                 "IO = IO0;",
only in patch2:
unchanged:
--- compiler/modules.m	4 Nov 2005 03:40:53 -0000	1.355
+++ compiler/modules.m	7 Nov 2005 07:42:56 -0000
@@ -2099,7 +2099,7 @@
 pragma_allowed_in_interface(foreign_decl(_, _, _), no).
 pragma_allowed_in_interface(foreign_import_module(_, _), yes).
 pragma_allowed_in_interface(foreign_code(_, _), no).
-pragma_allowed_in_interface(foreign_proc(_, _, _, _, _, _), no).
+pragma_allowed_in_interface(foreign_proc(_, _, _, _, _, _, _), no).
 pragma_allowed_in_interface(inline(_, _), no).
 pragma_allowed_in_interface(no_inline(_, _), no).
 pragma_allowed_in_interface(obsolete(_, _), yes).
@@ -5820,7 +5820,7 @@
         Info = Info0 ^ used_foreign_languages :=
             set__insert(Info0 ^ used_foreign_languages, Lang)
     ;
-        Pragma = foreign_proc(Attrs, Name, _, _, _, _)
+        Pragma = foreign_proc(Attrs, Name, _, _, _, _, _)
     ->
         NewLang = foreign_language(Attrs),
         ( OldLang = Info0 ^ foreign_proc_languages ^ elem(Name) ->
@@ -7388,7 +7388,7 @@
     Lang = foreign_type_language(ForeignType).
 item_needs_foreign_imports(pragma(_, foreign_decl(Lang, _, _)), Lang).
 item_needs_foreign_imports(pragma(_, foreign_code(Lang, _)), Lang).
-item_needs_foreign_imports(pragma(_, foreign_proc(Attrs, _, _, _, _, _)),
+item_needs_foreign_imports(pragma(_, foreign_proc(Attrs, _, _, _, _, _, _)),
         foreign_language(Attrs)).
 item_needs_foreign_imports(mutable(_, _, _, _, _), Lang) :-
     foreign_language(Lang).
@@ -7688,7 +7688,7 @@
     ; Pragma = foreign_code(_, _), Reorderable = no
     ; Pragma = foreign_decl(_, _, _), Reorderable = no
     ; Pragma = foreign_import_module(_, _), Reorderable = no
-    ; Pragma = foreign_proc(_, _, _, _, _, _), Reorderable = no
+    ; Pragma = foreign_proc(_, _, _, _, _, _, _), Reorderable = no
     ; Pragma = import(_, _, _, _, _), Reorderable = no
     ; Pragma = inline(_, _), Reorderable = yes
     ; Pragma = mode_check_clauses(_, _), Reorderable = yes
@@ -7776,7 +7776,7 @@
     ; Pragma = foreign_code(_, _), Reorderable = no
     ; Pragma = foreign_decl(_, _, _), Reorderable = no
     ; Pragma = foreign_import_module(_, _), Reorderable = no
-    ; Pragma = foreign_proc(_, _, _, _, _, _), Reorderable = no
+    ; Pragma = foreign_proc(_, _, _, _, _, _, _), Reorderable = no
     ; Pragma = import(_, _, _, _, _), Reorderable = no
     ; Pragma = inline(_, _), Reorderable = yes
     ; Pragma = mode_check_clauses(_, _), Reorderable = yes
only in patch2:
unchanged:
--- compiler/prog_data.m	4 Nov 2005 03:40:54 -0000	1.145
+++ compiler/prog_data.m	7 Nov 2005 07:34:31 -0000
@@ -427,6 +427,7 @@
                 proc_p_or_f             :: pred_or_func,
                 proc_vars               :: list(pragma_var),
                 proc_varset             :: prog_varset,
+                proc_instvarset         :: inst_varset,
                 proc_impl               :: pragma_foreign_code_impl
                 % Set of foreign proc attributes, eg.:
                 %   what language this code is in
only in patch2:
unchanged:
--- compiler/prog_io_pragma.m	28 Oct 2005 02:10:30 -0000	1.92
+++ compiler/prog_io_pragma.m	7 Nov 2005 07:45:02 -0000
@@ -1583,9 +1583,10 @@
         parse_pragma_c_code_varlist(VarSet0, VarList, PragmaVars, Error),
         (
             Error = no,
-            varset__coerce(VarSet0, VarSet),
+            varset__coerce(VarSet0, ProgVarSet),
+            varset__coerce(VarSet0, InstVarSet),
             Result = ok(pragma(user, foreign_proc(Flags, PredName, PredOrFunc,
-                PragmaVars, VarSet, PragmaImpl)))
+                PragmaVars, ProgVarSet, InstVarSet, PragmaImpl)))
         ;
             Error = yes(ErrorMessage),
             Result = error(ErrorMessage, PredAndVarsTerm0)
only in patch2:
unchanged:
--- compiler/recompilation.version.m	4 Nov 2005 03:40:55 -0000	1.30
+++ compiler/recompilation.version.m	7 Nov 2005 07:46:04 -0000
@@ -559,7 +559,7 @@
 is_pred_pragma(foreign_decl(_, _, _), no).
 is_pred_pragma(foreign_import_module(_, _), no).
 is_pred_pragma(foreign_code(_, _), no).
-is_pred_pragma(foreign_proc(_, Name, PredOrFunc, Args, _, _),
+is_pred_pragma(foreign_proc(_, Name, PredOrFunc, Args, _, _, _),
         yes(yes(PredOrFunc) - Name / Arity)) :-
     adjust_func_arity(PredOrFunc, Arity, list__length(Args)).
 is_pred_pragma(type_spec(Name, _, Arity, MaybePredOrFunc, _, _, _, _),
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------



More information about the reviews mailing list