[m-rev.] For review: add --simplify-stage-215 and extra information to the exception seen in bug90.

[Paul Bone]

On Fri, Feb 27, 2009 at 03:28:53PM +1100, Julien Fischer wrote:
> 
> On Fri, 27 Feb 2009, Paul Bone wrote:
> 
> >
> >For review by anyone.
> >
> >Estimated hours taken: 1.0
> >Branches: main
> >
> >In the case of an inconsitent rtti_type_infos error print the variable and 
> >the
> 
> s/inconsitent/inconsistent/
> 

Fixed.

> >rtti_type_infos that cause this error.
> >
> >Add an option to allow an explicit simplification pass to occur at stage 
> >215
> >even when profiling is not being used, this is helpful in detecting bugs in
> >generated code before the dead procedure elimination removes them.
> 
> I suggest:
> 
> 	Add an option that forces the pre-profiling simplificatation
> 	pass at stage 215 to be run, even when profiling is not be used.
> 	This is helpful ...

Fixed, thanks.

> >@@ -639,7 +640,9 @@ apply_substs_to_type_map(TRenaming, TSub
> >        ( Type = ExistingType ->
> >            true
> >        ;
> >-            unexpected(this_file, "inconsistent type_infos")
> >+            unexpected(this_file, string.format("inconsistent type_infos: 
> >"
> >+                ++ " Type: %s ExistingType: %s Var0: %s",
> >+                [s(string(Type)), s(string(ExistingType)), 
> >s(string(Var0))]))
> >        )
> >    ;
> >        svmap.det_insert(Var, Type, !Map)
> 
> In my opinion there is no point printing out the third of these.
> It is too much information for users and too little (and in most
> cases the wrong) information for developers.

This is probably true (I havn't thought about it much), I've fixed it,
it now only prints out the rtti_infos

 
> >Index: compiler/options.m
> >===================================================================
> >RCS file: /home/mercury1/repository/mercury/compiler/options.m,v
> >retrieving revision 1.643
> >diff -u -p -b -r1.643 options.m
> >--- compiler/options.m	20 Feb 2009 08:19:04 -0000	1.643
> >+++ compiler/options.m	26 Feb 2009 06:23:54 -0000
> >@@ -298,7 +298,12 @@
> >            % we only want to use the `yes' value, but we keep support for
> >            % the `no' value for benchmarks for the paper.
> >
> >-            % Perform coverage profiling, (enables deep profiling).
> >+    ;       simplify_stage_215
> >+            % Run the simplification pass at stage 215 (before profiling) 
> >this
> >+            % is implided by some of the profiling settings.
> 
> 
> s/implided/implied/
> 
> You also want to say that the option runs it, even when deep profiling
> is not enabled.
> 
> I wouldn't name the option after the stage number.  I thank that
> simplification pass has a name, pre_prof_transforms or something, so
> use that instead, e.g.
> 
> 	--pre-prof-transform-simplify
> 

This is a good idea, I'll do this.  I've named it
--pre-prof-transforms-simplify since that is the constently used name.
the hlds dump file was named pre-prof-transform-simplify, I've altered
this so that it is constentant (transforms is plural).

BTW: Do we use the stage numbers at all other than for sorting the hlds
dump files lexographicaly.

> >+
> >+            % Perform coverage profiling, this affects only deep profiling
> >+            % grades.
> >    ;       coverage_profiling
> >    ;       coverage_profiling_via_calls
> >
> >@@ -1179,6 +1184,7 @@ option_defaults_2(compilation_model_opti
> >    profile_memory                      -   bool(no),
> >    profile_deep                        -   bool(no),
> >    use_activation_counts               -   bool(no),
> >+    simplify_stage_215                  -   bool(no),
> >    coverage_profiling                  -   bool(no),
> >    coverage_profiling_via_calls        -   bool(no),
> >    profile_deep_coverage_after_goal    -   bool(yes),
> >@@ -2018,6 +2024,7 @@ long_option("profile-time",         prof
> >long_option("profile-memory",       profile_memory).
> >long_option("profile-deep",         profile_deep).
> >long_option("use-activation-counts",    use_activation_counts).
> >+long_option("simplify-stage-215",   simplify_stage_215).
> >long_option("coverage-profiling",
> >                    coverage_profiling).
> >long_option("coverage-profiling-via-calls",
> 
> There should be commented out documentation for the new options as well.
> (It should be explained why it s commented out, e.g. because it is
> for developer-only use.)

Fixed.

> >Index: runtime/mercury_deep_profiling.h
> >===================================================================
> >RCS file: 
> >/home/mercury1/repository/mercury/runtime/mercury_deep_profiling.h,v
> >retrieving revision 1.21
> >diff -u -p -b -r1.21 mercury_deep_profiling.h
> >--- runtime/mercury_deep_profiling.h	20 Sep 2008 11:38:05 -0000	1.21
> >+++ runtime/mercury_deep_profiling.h	19 Feb 2009 06:29:55 -0000
> >@@ -1,4 +1,7 @@
> >/*
> >+** vim: ts=8 sw=8 noexpandtab
> >+*/
> >+/*
> >** Copyright (C) 2001-2004, 2006-2008 The University of Melbourne.
> >** This file may only be copied under the terms of the GNU Library General
> >** Public License - see the file COPYING.LIB in the Mercury distribution.
> >
> 
> This change is not mentioned in the log message.

Whoops.  I don't intend to commit it, and it wasn't supposed to show up
for review.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20090302/8e186253/attachment.sig>