mercurylang/mercury
1
0
Fork 0
mirror of https://github.com/Mercury-Language/mercury.git synced 2025-12-12 20:34:19 +00:00
Code Issues Projects Releases Wiki Activity

Stop common_struct from interfering with mark_static_terms.

Browse Source 
This fixes Mantis bug #493.

compiler/common.m:
    Don't apply the common_struct optimization if doing so could possibly
    invalidate the annotations generated by mark_static_terms.m, which the
    MLDS code generator relies on.

    Move the tests for the applicability of the common_struct optimization
    for both construction and deconstruction unifications next to each other.

    Add XXXs about possible problems this code may have when applied to code
    that does region based memory allocation.

tests/valid/bug493.m:
    A simplified version of the Mantis test case.

tests/valid/Mmakefile:
tests/valid/Mercury.options:
    Enable the new test case, and specify the options that originally caused
    the bug to manifest itself.
This commit is contained in:
Zoltan Somogyi
2020-01-22 22:09:56 +11:00
parent 97fd7f5336
commit 18e222656f
4 changed files with 241 additions and 108 deletions
Download Patch File Download Diff File Expand all files Collapse all files

276
compiler/common.m
View File

@@ -305,23 +305,108 @@ common_info_clear_structs(!Info) :-
%---------------------------------------------------------------------------%
common_optimise_unification(Unification0, Mode, !GoalExpr, !GoalInfo,
common_optimise_unification(Unification0, UnifyMode, !GoalExpr, !GoalInfo,
!Common, !Info) :-
(
Unification0 = construct(Var, ConsId, ArgVars, _, _, _, SubInfo),
Unification0 = construct(Var, ConsId, ArgVars, _, How, _, SubInfo),
( if
SubInfo = construct_sub_info(MaybeTakeAddr, _),
MaybeTakeAddr = yes(_)
% There are three tests we have to pass before we even try
% to optimize away a construction. All three usually pass,
% so the order in which we test for them does not matter much.
% The first test is that none of the arguments should have
% their addresses taken. This is because the address being taken
% signifies that the value being put into the argument now
% is only a dummy, with the real value being supplied later
% (as can happen with code that has been optimized with
% last-call-modulo-construction).
(
SubInfo = no_construct_sub_info
;
SubInfo = construct_sub_info(MaybeTakeAddr, _),
MaybeTakeAddr = no
),
% The second test is that we don't optimise partially instantiated
% construction unifications, because it would be tricky to work out
% how to mode the replacement assignment unifications. In the
% vast majority of cases, the variable is ground.
simplify_info_get_module_info(!.Info, ModuleInfo),
UnifyMode = unify_modes_li_lf_ri_rf(_, LVarFinalInst, _, _),
inst_is_ground(ModuleInfo, LVarFinalInst),
% The third test is that mark_static_terms.m should not have
% already decided that we construct Var statically. This is because
% if it has, then it may have *also* decided that a term where
% Var occurs on the right hand side should *also* be constructed
% statically. If we replace the static construction of Var with an
% assign to Var from a coincidentally-guaranteed-to-be-identical
% term from somewhere else, as in tests/valid/bug493.m, then Var
% won't be marked as a static term in the MLDS code generator
% (the only backend that invokes mark_static_terms.m.), and
% we get a compiler abort when we get to the occurrence of Var
% on the right hand side of the later term.
%
% Note that the problem cannot happen in the LLDS code generator,
% since that backend does not use mark_static_terms.m, which means
% that if we replace the construction of Var by an assignment,
% it won't try to put terms containing Var into static data.
%
% Note also that this can happen *only* in procedure bodies
% that have been modified after semantic analysis, e.g. by
% inlining. This is because
%
% - we can see How = construct_statically only *after* the
% mark_static_terms pass has been run, which is way after
% the first simplification pass, which is run just after
% semantic analysis;
%
% - the common struct optimization we are implementing here
% is idempotent, so it can find new optimization opportunities
% on its second invocation only if the code has been modified
% after its first invocation.
%
% XXX This is only an instance of a more general problem.
% We should replace X = f(...) with X = Y *only* if the location
% of Y in terms of what memory area it is in (the heap, static
% data, or a region) satisfies the constraints imposed by the code
% that deals with X.
%
% Traditionally, except for the third test, the code we use here
% has worked in the usual case where How says that Var should be
% constructed either dynamically (on the heap) or statically.
% However, I (zs) have grave doubts about whether it does
% the right thing when either X or Y is supposed to be allocated
% in a region. This is because (a) the optimization is valid
% only if X and Y are supposed to be allocated from the *same*
% region; and (b) common_optimise_deconstruct does not record
% anything about Y, so we cannot possibly test for that here.
How = construct_dynamically
then
true
else
common_optimise_construct(Var, ConsId, ArgVars, Mode,
common_optimise_construct(Var, ConsId, ArgVars, LVarFinalInst,
!GoalExpr, !GoalInfo, !Common, !Info)
else
true
)
;
Unification0 = deconstruct(Var, ConsId, ArgVars, ArgModes, CanFail, _),
common_optimise_deconstruct(Var, ConsId, ArgVars, ArgModes, CanFail,
Mode, !GoalExpr, !GoalInfo, !Common, !Info)
UnifyMode = unify_modes_li_lf_ri_rf(LVarInitInst, _, _, _),
simplify_info_get_module_info(!.Info, ModuleInfo),
( if
% Don't optimise partially instantiated deconstruction
% unifications, because it would be tricky to work out
% how to mode the replacement assignment unifications.
% In the vast majority of cases, the variable is ground.
inst_is_ground(ModuleInfo, LVarInitInst)
% XXX See the comment on the "How = construct_dynamically" test
% above.
then
common_optimise_deconstruct(Var, ConsId, ArgVars, ArgModes,
CanFail, !GoalExpr, !GoalInfo, !Common, !Info)
else
true
)
;
( Unification0 = assign(Var1, Var2)
; Unification0 = simple_test(Var1, Var2)
@@ -332,127 +417,102 @@ common_optimise_unification(Unification0, Mode, !GoalExpr, !GoalInfo,
).
:- pred common_optimise_construct(prog_var::in, cons_id::in,
list(prog_var)::in, unify_mode::in,
list(prog_var)::in, mer_inst::in,
hlds_goal_expr::in, hlds_goal_expr::out,
hlds_goal_info::in, hlds_goal_info::out,
common_info::in, common_info::out,
simplify_info::in, simplify_info::out) is det.
common_optimise_construct(Var, ConsId, ArgVars, UnifyMode, GoalExpr0, GoalExpr,
GoalInfo0, GoalInfo, !Common, !Info) :-
simplify_info_get_module_info(!.Info, ModuleInfo),
UnifyMode = unify_modes_li_lf_ri_rf(_, LVarFinalInst, _, _),
% Don't optimise partially instantiated construction unifications,
% because it would be tricky to work out how to mode the replacement
% assignment unifications. In the vast majority of cases, the variable
% is ground.
( if inst_is_ground(ModuleInfo, LVarFinalInst) then
TypeCtor = lookup_var_type_ctor(!.Info, Var),
VarEqv0 = !.Common ^ var_eqv,
list.map_foldl(eqvclass.ensure_element_partition_id,
ArgVars, ArgVarIds, VarEqv0, VarEqv1),
AllStructMap0 = !.Common ^ all_structs,
( if
% generate_assign assumes that the output variable
% is in the instmap_delta, which will not be true if the
% variable is local to the unification. The optimization
% is pointless in that case.
InstMapDelta = goal_info_get_instmap_delta(GoalInfo0),
instmap_delta_search_var(InstMapDelta, Var, _),
common_optimise_construct(Var, ConsId, ArgVars, LVarFinalInst,
GoalExpr0, GoalExpr, GoalInfo0, GoalInfo, !Common, !Info) :-
TypeCtor = lookup_var_type_ctor(!.Info, Var),
VarEqv0 = !.Common ^ var_eqv,
list.map_foldl(eqvclass.ensure_element_partition_id,
ArgVars, ArgVarIds, VarEqv0, VarEqv1),
AllStructMap0 = !.Common ^ all_structs,
( if
% generate_assign assumes that the output variable is in the
% instmap_delta, which will not be true if the variable is local
% to the unification. The optimization is pointless in that case.
InstMapDelta = goal_info_get_instmap_delta(GoalInfo0),
instmap_delta_search_var(InstMapDelta, Var, _),
map.search(AllStructMap0, TypeCtor, ConsIdMap0),
map.search(ConsIdMap0, ConsId, Structs),
find_matching_cell_construct(Structs, VarEqv1, ArgVarIds,
OldStruct)
then
OldStruct = structure(OldVar, _),
eqvclass.ensure_equivalence(Var, OldVar, VarEqv1, VarEqv),
!Common ^ var_eqv := VarEqv,
(
ArgVars = [],
% Constants don't use memory, so there is no point in
% optimizing away their construction; in fact, doing so
% could cause more stack usage.
GoalExpr = GoalExpr0,
GoalInfo = GoalInfo0
;
ArgVars = [_ | _],
VarFromToInsts = from_to_insts(LVarFinalInst, LVarFinalInst),
generate_assign(Var, OldVar, VarFromToInsts, GoalInfo0,
GoalExpr, GoalInfo, !Common, !Info),
simplify_info_set_should_requantify(!Info),
goal_cost(hlds_goal(GoalExpr0, GoalInfo0), Cost),
simplify_info_incr_cost_delta(Cost, !Info)
)
else
map.search(AllStructMap0, TypeCtor, ConsIdMap0),
map.search(ConsIdMap0, ConsId, Structs),
find_matching_cell_construct(Structs, VarEqv1, ArgVarIds, OldStruct)
then
OldStruct = structure(OldVar, _),
eqvclass.ensure_equivalence(Var, OldVar, VarEqv1, VarEqv),
!Common ^ var_eqv := VarEqv,
(
ArgVars = [],
% Constants don't use memory, so there is no point in
% optimizing away their construction; in fact, doing so
% could cause more stack usage.
GoalExpr = GoalExpr0,
GoalInfo = GoalInfo0,
Struct = structure(Var, ArgVars),
record_cell_in_maps(TypeCtor, ConsId, Struct, VarEqv1, !Common)
GoalInfo = GoalInfo0
;
ArgVars = [_ | _],
VarFromToInsts = from_to_insts(LVarFinalInst, LVarFinalInst),
generate_assign(Var, OldVar, VarFromToInsts, GoalInfo0,
GoalExpr, GoalInfo, !Common, !Info),
simplify_info_set_should_requantify(!Info),
goal_cost(hlds_goal(GoalExpr0, GoalInfo0), Cost),
simplify_info_incr_cost_delta(Cost, !Info)
)
else
GoalExpr = GoalExpr0,
GoalInfo = GoalInfo0
GoalInfo = GoalInfo0,
Struct = structure(Var, ArgVars),
record_cell_in_maps(TypeCtor, ConsId, Struct, VarEqv1, !Common)
).
:- pred common_optimise_deconstruct(prog_var::in, cons_id::in,
list(prog_var)::in, list(unify_mode)::in, can_fail::in, unify_mode::in,
list(prog_var)::in, list(unify_mode)::in, can_fail::in,
hlds_goal_expr::in, hlds_goal_expr::out,
hlds_goal_info::in, hlds_goal_info::out,
common_info::in, common_info::out,
simplify_info::in, simplify_info::out) is det.
common_optimise_deconstruct(Var, ConsId, ArgVars, ArgModes, CanFail, UnifyMode,
common_optimise_deconstruct(Var, ConsId, ArgVars, ArgModes, CanFail,
GoalExpr0, GoalExpr, GoalInfo0, GoalInfo, !Common, !Info) :-
simplify_info_get_module_info(!.Info, ModuleInfo),
TypeCtor = lookup_var_type_ctor(!.Info, Var),
VarEqv0 = !.Common ^ var_eqv,
eqvclass.ensure_element_partition_id(Var, VarId, VarEqv0, VarEqv1),
SinceCallStructMap0 = !.Common ^ since_call_structs,
( if
% Don't optimise partially instantiated deconstruction unifications,
% because it would be tricky to work out how to mode the replacement
% assignment unifications. In the vast majority of cases, the variable
% is ground.
UnifyMode = unify_modes_li_lf_ri_rf(LVarInitInst, _, _, _),
not inst_is_ground(ModuleInfo, LVarInitInst)
then
GoalExpr = GoalExpr0
else
TypeCtor = lookup_var_type_ctor(!.Info, Var),
VarEqv0 = !.Common ^ var_eqv,
eqvclass.ensure_element_partition_id(Var, VarId, VarEqv0, VarEqv1),
SinceCallStructMap0 = !.Common ^ since_call_structs,
( if
% Do not delete deconstruction unifications inserted by
% stack_opt.m or tupling.m, which have done a more comprehensive
% cost analysis than common.m can do.
not goal_info_has_feature(GoalInfo, feature_stack_opt),
not goal_info_has_feature(GoalInfo, feature_tuple_opt),
% Do not delete deconstruction unifications inserted by
% stack_opt.m or tupling.m, which have done a more comprehensive
% cost analysis than common.m can do.
not goal_info_has_feature(GoalInfo, feature_stack_opt),
not goal_info_has_feature(GoalInfo, feature_tuple_opt),
map.search(SinceCallStructMap0, TypeCtor, ConsIdMap0),
map.search(ConsIdMap0, ConsId, Structs),
find_matching_cell_deconstruct(Structs, VarEqv1, VarId, OldStruct)
then
OldStruct = structure(_, OldArgVars),
eqvclass.ensure_corresponding_equivalences(ArgVars,
OldArgVars, VarEqv1, VarEqv),
!Common ^ var_eqv := VarEqv,
RHSFromToInsts = list.map(unify_modes_to_rhs_from_to_insts,
ArgModes),
create_output_unifications(GoalInfo0, ArgVars, OldArgVars,
RHSFromToInsts, Goals, !Common, !Info),
GoalExpr = conj(plain_conj, Goals),
goal_cost(hlds_goal(GoalExpr0, GoalInfo0), Cost),
simplify_info_incr_cost_delta(Cost, !Info),
simplify_info_set_should_requantify(!Info),
(
CanFail = can_fail,
simplify_info_set_should_rerun_det(!Info)
;
CanFail = cannot_fail
)
else
GoalExpr = GoalExpr0,
Struct = structure(Var, ArgVars),
record_cell_in_maps(TypeCtor, ConsId, Struct, VarEqv1, !Common)
map.search(SinceCallStructMap0, TypeCtor, ConsIdMap0),
map.search(ConsIdMap0, ConsId, Structs),
find_matching_cell_deconstruct(Structs, VarEqv1, VarId, OldStruct)
then
OldStruct = structure(_, OldArgVars),
eqvclass.ensure_corresponding_equivalences(ArgVars,
OldArgVars, VarEqv1, VarEqv),
!Common ^ var_eqv := VarEqv,
RHSFromToInsts = list.map(unify_modes_to_rhs_from_to_insts,
ArgModes),
create_output_unifications(GoalInfo0, ArgVars, OldArgVars,
RHSFromToInsts, Goals, !Common, !Info),
GoalExpr = conj(plain_conj, Goals),
goal_cost(hlds_goal(GoalExpr0, GoalInfo0), Cost),
simplify_info_incr_cost_delta(Cost, !Info),
simplify_info_set_should_requantify(!Info),
(
CanFail = can_fail,
simplify_info_set_should_rerun_det(!Info)
;
CanFail = cannot_fail
)
else
GoalExpr = GoalExpr0,
Struct = structure(Var, ArgVars),
record_cell_in_maps(TypeCtor, ConsId, Struct, VarEqv1, !Common)
),
GoalInfo = GoalInfo0.

1
tests/valid/Mercury.options
View File

@@ -41,6 +41,7 @@ MCFLAGS-bug271 = --allow-stubs --no-warn-stubs
MCFLAGS-bug300 = --optimize-constructor-last-call
MCFLAGS-bug457 = --loop-invariants --intermodule-optimization
MCFLAGS-bug483 = --warn-unused-imports --halt-at-warn
MCFLAGS-bug493 = --loop-invariants
MCFLAGS-compl_unify_bug = -O3
MCFLAGS-constraint_prop_bug = -O0 --common-struct --local-constraint-propagation
MCFLAGS-csharp_hello = --no-intermodule-optimization

1
tests/valid/Mmakefile
View File

@@ -91,6 +91,7 @@ OTHER_PROGS = \
bug481 \
bug483 \
bug486 \
bug493 \
bug51 \
bug85 \
builtin_false \

71
tests/valid/bug493.m Normal file
View File

@@ -0,0 +1,71 @@
%---------------------------------------------------------------------------%
% vim: ts=2 sw=2 ft=mercury
%---------------------------------------------------------------------------%
%
% This is a regression test for Mantis bug #493.
%
% The bug was a compiler abort in the MLDS code generator when processing
% the binding of Range in the parse predicate. The sequence of events
% leading to this abort was as follows.
%
% 1 The compiler inlines dash in parse_range.
% 2 The compiler inlines parse_range (with dash inlined) in parse.
% 3 The mark_static_terms pass marks both of the construction unifications
% involved in Range = range(unicode(0x2d)) as being done statically.
% 4 The mark_static_terms pass marks the construction unification
% Res = [Range] as also being done statically.
% 5 The pre-code-generation invocation of simplification runs the common
% structure optimization on the body of parse, and replaces the code
% that constructs Range with code that assigns the unnamed internal
% variable representing the first element of Us0 to Range, exploiting
% the coincidence that both are bound to unicode(0x2d).
% 6 When the MLDS code generator processes that assignment to Range,
% it does not mark Range is being statically constructed, because
% it isn't.
% 7 When the MLDS code generator processes the construction unification
% that implements Res = [Range], it does so using the code path that
% constructs Res statically, since mark_static_terms said that Res
% should be constructed statically. However, this requires all the variables
% on the right hand side of the construction unification to be in the
% database of statically constructed terms, which (due to steps 5 and 6
% above) it is not. This violation of this code path's precondition
% causes the abort.
%
% The actual culprit is step 5. By replacing a "construct statically"
% construction unification of Range with an assignment, it broke all later
% "construct statically" construction unifications in which Range occurred
% on the right hand side.
%
%---------------------------------------------------------------------------%
:- module bug493.
:- interface.
:- import_module list.
:- type range
---> range(unicode).
:- type unicode
---> unicode(int).
:- pred parse(list(unicode)::in, list(unicode)::out, list(range)::out)
is semidet.
:- implementation.
parse(Us0, Us, Res) :-
parse_range(Us0, Us, Range),
Res = [Range].
:- pred parse_range(list(unicode)::in, list(unicode)::out, range::out)
is semidet.
parse_range(Us0, Us, Range) :-
Us0 = [unicode(0x2d) | Us],
Range = dash.
:- func dash = range.
:- pragma inline(dash/0).
dash = range(unicode(0x2d)).