[m-rev.] for review: Java unreachable code removal Mk.2

[Fergus Henderson]

On 20-Feb-2002, Michael Wybrow <mjwybrow at students.cs.mu.oz.au> wrote:
> 
> Do not generate unreachable code in the Java back-end, and re-enable 
> optimized tailcalls which were previously generating unreachable code.
...
> Index: mlds_to_java.m
> @@ -1780,33 +1789,66 @@
...
> +	% These types are used by many of the output_stmt style predicates to
> +	% pass back completetion information for previous statements.

s/etet/et/

But it's not clear what you mean by "completion information" here --
it would be better to explain in more detail.  E.g. something like
this would be better:

	% These types are used by many of the output_stmt style predicates to
	% return information about the statement's control flow,
	% i.e. about the different ways in which the statement can exit.

> +:- type jump_type
> +	--->	can_break
> +	;	can_continue
> +	;	can_return
> +	;	can_throw
> +	;	can_complete
> +	.

The meaning of "can_complete" is not clear, and should be explained.
It might be better to rename this as "can_fall_through".

Also "can_complete" is not a type of jump, so naming this type
"jump_type" is a bit misleading.  "exit_method" would be better.

Likewise "JumpInfo" => "ExitMethods".

> +while_jump_info(Cond, BlockJumpInfo) = JumpInfo :-
> +	% A while statement cannot complete normally if its condition
> +	% expression is a constant expression with value true, and it
> +	% doesn't contain a reachable break statement that exits the
> +	% while statement.
> +	(
> +		Cond = mlds__const(mlds__true),

There should be an XXX comment here, since `mlds__const'
is not a sufficient way of testing for a Java "constant expression".

> +		not set__member(can_break, BlockJumpInfo)
> +	->
> +		% Cannot complete normally
> +		JumpInfo = set__init

It's wrong to delete "can_throw" and "can_return" here.

Actually I think your code never tests whether the JumpInfo contains
"can_throw" or "can_return", so it probably doesn't matter too much,
but you might as well get it right...

> +	;
> +		JumpInfo0 = set__delete(BlockJumpInfo, can_continue),
> +		JumpInfo1 = set__delete(JumpInfo0, can_break),
> +		JumpInfo  = set__insert(JumpInfo1, can_complete)
> +	).

It might be clearer to write that as

	JumpInfo = ((BlockJumpInfo `set__delete` can_continue)
			           `set__delete` can_break)
			           `set__insert` can_complete

>  	%
>  	% selection (if-then-else)
>  	%
>  output_stmt(Indent, FuncInfo, if_then_else(Cond, Then0, MaybeElse),
> -		Context) -->
> +		Context, JumpInfo) -->
>  	%
>  	% we need to take care to avoid problems caused by the
>  	% dangling else ambiguity
> @@ -1876,55 +1955,67 @@
>  	io__write_string("if ("),
>  	output_rval(Cond),
>  	io__write_string(")\n"),
> -	output_statement(Indent + 1, FuncInfo, Then),
> +	output_statement(Indent + 1, FuncInfo, Then, ThenJumpInfo),
>  	( { MaybeElse = yes(Else) } ->
>  		indent_line(Context, Indent),
>  		io__write_string("else\n"),
> -		output_statement(Indent + 1, FuncInfo, Else)
> -	;
> -		[]
> +		output_statement(Indent + 1, FuncInfo, Else, ElseJumpInfo),
> +		% An if-then-else statement can complete normally iff the 
> +		% then-statement can complete normally or the else-statement
> +		% can complete normally.
> +		{ JumpInfo = set__union(ThenJumpInfo, ElseJumpInfo) }
> +	;
> +		% An if-then statement can complete normally iff it is 
> +		% reachable.
> +		{ JumpInfo = set__make_singleton_set(can_complete) }
>  	).

This is wrong.  You've thrown away the possibility that the `then'
part could break, continue, throw, or return.  It should be

		{ JumpInfo = ThenJumpInfo `set__union` 
				set__make_singleton_set(can_complete) }

>  	%
>  	% transfer of control
>  	% 
> -output_stmt(_Indent, _FuncInfo, label(_LabelName), _Context) --> 
> +output_stmt(_Indent, _FuncInfo, label(_LabelName), _Context, set__init) --> 
>  	{ unexpected(this_file, 
>  		"output_stmt: labels not supported in Java.") }.

s/set__init/_JumpInfo/

`set__init' would be wrong, and there's no need to lie;
using an underscore variable here is fine.

> +output_stmt(_Indent, _FuncInfo, goto(label(_LabelName)), _Context,
> +		set__init) --> 
>  	{ unexpected(this_file,
>  		"output_stmt: gotos not supported in Java.") }.

Likewise.

> +output_stmt(_Indent, _FuncInfo, computed_goto(_Expr, _Labels), _Context,
> +		set__init) --> 
>  	{ unexpected(this_file, 
>  		"output_stmt: computed gotos not supported in Java.") }.

Likewise.

> @@ -2035,16 +2126,13 @@
>  	;
>  		[]
>  	),
> -	% XXX Is this needed? If present, it causes compiler errors for a
> -	%     couple of files in the benchmarks directory.  -mjwybrow
> -	%
> -	% ( { IsTailCall = tail_call, Results = [] } ->
> -	%	indent_line(Context, Indent + 1),
> -	%	io__write_string("return;\n")
> -	% ;
> -	%	[]
> -	% ),
> -	%
> +	( { IsTailCall = tail_call, Results = [] } ->
> +		indent_line(Context, Indent + 1),
> +		io__write_string("return;\n"),
> +		{ JumpInfo = set__make_singleton_set(can_return) }
> +	;
> +		{ JumpInfo = set__make_singleton_set(can_complete) }
> +	),

This change is not mentioned in the log message.  It's also wrong -- you
need to check the return type of the calling function before inserting a
"return;" statement.

For this change I suggest just leaving this commented out with an XXX
as it was before.  It can be fixed as a separate change.

> +output_switch_default(Indent, _FuncInfo, Context, default_is_unreachable,
> +		JumpInfo) -->
>  	indent_line(Context, Indent),
>  	io__write_string("default: /*NOTREACHED*/\n"), 
>  	indent_line(Context, Indent + 1),
> -	io__write_string("throw new mercury.runtime.UnreachableDefault();\n").
> +	io__write_string("throw new mercury.runtime.UnreachableDefault();\n"),
> +	% This exception will never be caught so statements following it are
> +	% unreachable.
> +	{ JumpInfo = set__make_singleton_set(can_throw) }.

The comment here is not helpful.  Firstly, the exception might well get caught.
Secondly, the statements following the throw are unreachable regardless of
whether or not the exception gets caught.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------