[m-rev.] diff: fix bug in jumpopt.m
Zoltan Somogyi
zs at cs.mu.OZ.AU
Thu Jun 16 13:44:18 AEST 2005
compiler/jumpopt.m:
Fix a big in jumpopt exposed by my recent change to pragma_c_gen.m.
Some of the transformations eliminate labels which are followed
immediately by a goto to another label. This is safe only if all
referenced to the eliminated label are replaced by a reference to
the other label. The bug was that the old code of this module wasn't
thorough in performing this replacement, missing out on the labels
in pragma_c instructions. The fix is to short-circuit labels in
pragma_c instructions, and to convert the relevant predicate to a
switch to ensure no more occurrences of the bug. This conversion
discovered another instance of the bug: we weren't short-circuiting
labels in fork instructions. (This would show up only in LLDS .par
grades.)
Julien's workaround for the bug will be removed when this diff has been
installed on all our systems.
Zoltan.
Index: jumpopt.m
===================================================================
RCS file: /home/mercury/mercury1/repository/mercury/compiler/jumpopt.m,v
retrieving revision 1.75
diff -u -b -r1.75 jumpopt.m
--- jumpopt.m 16 Jun 2005 03:38:15 -0000 1.75
+++ jumpopt.m 16 Jun 2005 03:38:25 -0000
@@ -259,10 +259,16 @@
Lvalmap, Procmap, Sdprocmap, Forkmap, Succmap, LayoutLabels,
Fulljumpopt, MayAlterRtti, !CheckedNondetTailCallInfo, Instrs) :-
Instr0 = Uinstr0 - Comment0,
+ % We do a switch on the instruction type to ensure that we short circuit
+ % all the labels that are in Instrmap but not in LayoutLabels in *all*
+ % instructions in which they occur. This means we must fully search
+ % every part of every instruction that may possibly hold a label.
+ % In theory, this means every lval and every rval, but in practice we
+ % know that rvals representing e.g. the tags of fields cannot contain
+ % labels.
(
- Uinstr0 = call(Proc, label(RetLabel), LiveInfos, Context,
- GoalPath, CallModel)
- ->
+ Uinstr0 = call(Proc, RetAddr, LiveInfos, Context, GoalPath, CallModel),
+ ( RetAddr = label(RetLabel) ->
(
% Look for det style tailcalls. We look for this
% even if the call is semidet because one of the
@@ -324,7 +330,8 @@
counter__allocate(LabelNum, Counter0, Counter1),
NewLabel = internal(LabelNum, ProcLabel),
NewInstrs = [
- if_val(binop(ne, lval(curfr), lval(maxfr)), label(NewLabel))
+ if_val(binop(ne, lval(curfr), lval(maxfr)),
+ label(NewLabel))
- "branch around if cannot tail call",
assign(maxfr, lval(prevfr(lval(curfr))))
- "discard this frame",
@@ -351,8 +358,9 @@
NewInstrs = [Instr0],
RemainInstrs = Instrs0
;
- NewInstrs = [call(Proc, label(DestLabel), LiveInfos, Context,
- GoalPath, CallModel) - redirect_comment(Comment0)],
+ NewInstrs = [call(Proc, label(DestLabel), LiveInfos,
+ Context, GoalPath, CallModel)
+ - redirect_comment(Comment0)],
RemainInstrs = Instrs0
)
;
@@ -360,8 +368,12 @@
RemainInstrs = Instrs0
)
;
- Uinstr0 = goto(label(TargetLabel))
- ->
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ )
+ ;
+ Uinstr0 = goto(TargetAddr),
+ ( TargetAddr = label(TargetLabel) ->
(
% Eliminate the goto if possible.
opt_util__is_this_label_next(TargetLabel, Instrs0, _)
@@ -385,14 +397,14 @@
map__search(Procmap, TargetLabel, Between0)
->
jumpopt__adjust_livevals(PrevInstr, Between0, Between),
- list__append(Between, [goto(succip) - "shortcircuit"], NewInstrs),
+ NewInstrs = Between ++ [goto(succip) - "shortcircuit"],
RemainInstrs = Instrs0
;
% Replace a jump to a semidet epilog with the epilog.
map__search(Sdprocmap, TargetLabel, Between0)
->
jumpopt__adjust_livevals(PrevInstr, Between0, Between),
- list__append(Between, [goto(succip) - "shortcircuit"], NewInstrs),
+ NewInstrs = Between ++ [goto(succip) - "shortcircuit"],
RemainInstrs = Instrs0
;
% Replace a jump to a nondet epilog with the epilog.
@@ -423,7 +435,8 @@
block_may_be_duplicated(Block) = yes
->
opt_util__filter_out_labels(Block, FilteredBlock),
- jumpopt__adjust_livevals(PrevInstr, FilteredBlock, AdjustedBlock),
+ jumpopt__adjust_livevals(PrevInstr, FilteredBlock,
+ AdjustedBlock),
% Block may end with a goto to DestLabel. We avoid
% infinite expansion in such cases by removing
% DestLabel from Blockmap, though only while
@@ -441,12 +454,14 @@
jumpopt__final_dest(Instrmap, TargetLabel, DestLabel,
TargetInstr, DestInstr),
DestInstr = UdestInstr - _Destcomment,
- string__append("shortcircuited jump: ", Comment0, Shorted),
+ Shorted = "shortcircuited jump: " ++ Comment0,
opt_util__can_instr_fall_through(UdestInstr, Canfallthrough),
- ( Canfallthrough = no ->
+ (
+ Canfallthrough = no,
NewInstrs0 = [UdestInstr - Shorted],
RemainInstrs = Instrs0
;
+ Canfallthrough = yes,
( TargetLabel = DestLabel ->
NewInstrs0 = [Instr0],
RemainInstrs = Instrs0
@@ -456,8 +471,8 @@
)
),
( map__search(Lvalmap, DestLabel, yes(Lvalinstr)) ->
- jumpopt__adjust_livevals(PrevInstr, [Lvalinstr | NewInstrs0],
- NewInstrs)
+ jumpopt__adjust_livevals(PrevInstr,
+ [Lvalinstr | NewInstrs0], NewInstrs)
;
NewInstrs = NewInstrs0
)
@@ -466,20 +481,23 @@
RemainInstrs = Instrs0
)
;
- Uinstr0 = computed_goto(Index, LabelList0)
- ->
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ )
+ ;
+ Uinstr0 = computed_goto(Index, LabelList0),
% Short-circuit all the destination labels.
jumpopt__short_labels(Instrmap, LabelList0, LabelList),
RemainInstrs = Instrs0,
( LabelList = LabelList0 ->
NewInstrs = [Instr0]
;
- string__append(Comment0, " (some shortcircuits)", Shorted),
+ Shorted = Comment0 ++ " (some shortcircuits)",
NewInstrs = [computed_goto(Index, LabelList) - Shorted]
)
;
- Uinstr0 = if_val(Cond, label(TargetLabel))
- ->
+ Uinstr0 = if_val(Cond, TargetAddr),
+ ( TargetAddr = label(TargetLabel) ->
(
% Attempt to transform code such as
%
@@ -495,13 +513,14 @@
%
% The label L1 may be present or not. If it is present,
% we are eliminating it, which is possible only because
- % we can short-circuit jumps to it (make them jump
+ % we short-circuit all jumps to it (make them jump
% directly to L3). This may not be possible if L3 is
% a non-label code address; e.g. we cannot jump to
% non-label code addresses from computed gotos.
opt_util__skip_comments(Instrs0, Instrs1),
Instrs1 = [Instr1 | Instrs2],
- ( Instr1 = label(_) - _ ->
+ ( Instr1 = label(ElimLabel) - _ ->
+ not set__member(ElimLabel, LayoutLabels),
opt_util__skip_comments(Instrs2, Instrs3),
Instrs3 = [GotoInstr | AfterGoto],
HaveLabel = yes
@@ -586,8 +605,10 @@
opt_util__is_sdproceed_next(Instrs0, BetweenFT),
map__search(Blockmap, DestLabel, Block),
opt_util__is_sdproceed_next(Block, BetweenBR),
- opt_util__filter_out_r1(BetweenFT, yes(SuccessFT), Between),
- opt_util__filter_out_r1(BetweenBR, yes(SuccessBR), Between),
+ opt_util__filter_out_r1(BetweenFT, yes(SuccessFT),
+ Between),
+ opt_util__filter_out_r1(BetweenBR, yes(SuccessBR),
+ Between),
(
SuccessFT = true,
SuccessBR = false,
@@ -612,7 +633,7 @@
% Try to short-circuit the destination.
TargetLabel \= DestLabel
->
- string__append("shortcircuited jump: ", Comment0, Shorted),
+ Shorted = "shortcircuited jump: " ++ Comment0,
NewInstrs = [if_val(Cond, label(DestLabel)) - Shorted],
RemainInstrs = Instrs0
;
@@ -624,8 +645,11 @@
RemainInstrs = Instrs0
)
;
- Uinstr0 = assign(Lval, Rval0)
- ->
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ )
+ ;
+ Uinstr0 = assign(Lval, Rval0),
% Any labels mentioned in Rval0 should be short-circuited.
jumpopt__short_labels_rval(Instrmap, Rval0, Rval),
RemainInstrs = Instrs0,
@@ -636,19 +660,187 @@
NewInstrs = [assign(Lval, Rval) - Shorted]
)
;
- Uinstr0 = mkframe(FrameInfo, yes(label(Label0)))
- ->
+ Uinstr0 = mkframe(FrameInfo, Redoip),
+ ( Redoip = yes(label(Label0)) ->
jumpopt__short_label(Instrmap, Label0, Label),
RemainInstrs = Instrs0,
( Label = Label0 ->
NewInstrs = [Instr0]
;
- string__append(Comment0, " (some shortcircuits)", Shorted),
+ Shorted = Comment0 ++ " (some shortcircuits)",
NewInstrs = [mkframe(FrameInfo, yes(label(Label))) - Shorted]
)
;
NewInstrs = [Instr0],
RemainInstrs = Instrs0
+ )
+ ;
+ Uinstr0 = pragma_c(Decls, Components0, MayCallMercury,
+ MaybeFixNoLayout, MaybeFixLayout, MaybeFixOnlyLayout,
+ MaybeNoFix0, StackSlotRef, MaybeDup),
+ some [!Redirect] (
+ list__map_foldl(short_pragma_component(Instrmap),
+ Components0, Components, no, !:Redirect),
+ (
+ MaybeNoFix0 = yes(NoFix),
+ short_label(Instrmap, NoFix, NoFixDest),
+ MaybeNoFix = yes(NoFixDest),
+ !:Redirect = yes
+ ;
+ MaybeNoFix0 = no,
+ MaybeNoFix = no
+ ),
+% These sanity checks are too strong, because we don't prohibit labels
+% appearing these slots of pragma_c instructions from appearing in Instrmap;
+% we only prohibit the use of those entries in Instrmap to optimize away these
+% labels.
+%
+% (
+% MaybeFixNoLayout = yes(FixNoLayout),
+% short_label(Instrmap, FixNoLayout, FixNoLayoutDest),
+% FixNoLayoutDest \= FixNoLayout
+% ->
+% error("jumpopt__instr_list: pragma_c fix_no_layout")
+% ;
+% true
+% ),
+% (
+% MaybeFixLayout = yes(FixLayout),
+% short_label(Instrmap, FixLayout, FixLayoutDest),
+% FixLayoutDest \= FixLayout
+% ->
+% error("jumpopt__instr_list: pragma_c fix_layout")
+% ;
+% true
+% ),
+% (
+% MaybeFixOnlyLayout = yes(FixOnlyLayout),
+% short_label(Instrmap, FixOnlyLayout, FixOnlyLayoutDest),
+% FixOnlyLayoutDest \= FixOnlyLayout
+% ->
+% error("jumpopt__instr_list: pragma_c fix_only_layout")
+% ;
+% true
+% ),
+ (
+ !.Redirect = no,
+ Instr = Instr0
+ ;
+ !.Redirect = yes,
+ Comment = Comment0 ++ " (some redirects)",
+ Uinstr = pragma_c(Decls, Components, MayCallMercury,
+ MaybeFixNoLayout, MaybeFixLayout, MaybeFixOnlyLayout,
+ MaybeNoFix, StackSlotRef, MaybeDup),
+ Instr = Uinstr - Comment
+ )
+ ),
+ NewInstrs = [Instr],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = c_code(_, _),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = comment(_),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = livevals(_),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = block(_, _, _),
+ % These are supposed to be introduced only after jumpopt is run
+ % for the last time.
+ error("jumpopt__instr_list: block")
+ ;
+ Uinstr0 = label(_),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = incr_sp(_, _),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = decr_sp(_),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = store_ticket(_),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = reset_ticket(_, _),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = discard_ticket,
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = prune_ticket,
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = prune_tickets_to(_),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = mark_ticket_stack(_),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = mark_hp(_),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = free_heap(_),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = incr_hp(_, _, _, _, _),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = restore_hp(_),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = fork(Child0, Parent0, NumSlots),
+ short_label(Instrmap, Child0, Child),
+ short_label(Instrmap, Parent0, Parent),
+ (
+ Child = Child0,
+ Parent = Parent0
+ ->
+ Instr = Instr0
+ ;
+ Uinstr = fork(Child, Parent, NumSlots),
+ Comment = Comment0 ++ " (redirect)",
+ Instr = Uinstr - Comment
+ ),
+ NewInstrs = [Instr],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = init_sync_term(_, _),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = join_and_continue(SyncTerm, Label0),
+ short_label(Instrmap, Label0, Label),
+ ( Label = Label0 ->
+ Instr = Instr0
+ ;
+ Uinstr = join_and_continue(SyncTerm, Label),
+ Comment = Comment0 ++ " (redirect)",
+ Instr = Uinstr - Comment
+ ),
+ NewInstrs = [Instr],
+ RemainInstrs = Instrs0
+ ;
+ Uinstr0 = join_and_terminate(_),
+ NewInstrs = [Instr0],
+ RemainInstrs = Instrs0
),
( ( Uinstr0 = comment(_) ; NewInstrs = [] ) ->
NewPrevInstr = PrevInstr
@@ -897,5 +1089,36 @@
jumpopt__short_labels_rval(Instrmap, Rval0, Rval).
jumpopt__short_labels_lval(_, lvar(_), _) :-
error("lvar lval in jumpopt__short_labels_lval").
+
+:- pred short_pragma_component(instrmap::in,
+ pragma_c_component::in, pragma_c_component::out,
+ bool::in, bool::out) is det.
+
+short_pragma_component(Instrmap, Component0, Component, !Redirect) :-
+ (
+ Component0 = pragma_c_inputs(_),
+ Component = Component0
+ ;
+ Component0 = pragma_c_outputs(_),
+ Component = Component0
+ ;
+ Component0 = pragma_c_user_code(_, _),
+ Component = Component0
+ ;
+ Component0 = pragma_c_raw_code(_, _),
+ Component = Component0
+ ;
+ Component0 = pragma_c_fail_to(Label0),
+ short_label(Instrmap, Label0, Label),
+ Component = pragma_c_fail_to(Label),
+ ( Label = Label0 ->
+ true
+ ;
+ !:Redirect = yes
+ )
+ ;
+ Component0 = pragma_c_noop,
+ Component = Component0
+ ).
%-----------------------------------------------------------------------------%
--------------------------------------------------------------------------
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