[m-rev.] For review: Annotate the HLDS with regions

[Quan Phan]

Hi Julien,

Please read my answers between the lines. I will post the diff in another email.

Thanks and regards,
Quan

Quoting Julien Fischer <juliensf at csse.unimelb.edu.au>:

> > Annotate the HLDS with information about regions. This includes calls to
> > region builtins, extra region arguments for procedures and calls, regions
> > for construction unifications.
> 
> For a change of this size the log message should go into more detail

I did, we will see in the email with the diff.

> > +    ;
> > +            construct_in_region(prog_var)
> 
> Add a comment describing this alternative.

Added.

> 
> > Index: quantification.m
> > ===================================================================
> > RCS file:
> /home/mercury/mercury1/repository/mercury/compiler/quantification.m,v
> > retrieving revision 1.116
> > diff -u -u -r1.116 quantification.m
> > --- quantification.m	12 Jun 2007 07:21:25 -0000	1.116
> > +++ quantification.m	18 Jun 2007 12:37:41 -0000
> > @@ -517,6 +517,8 @@
> >         ;
> >             ( How = construct_statically(_)
> >             ; How = construct_dynamically
> > +            % XXX Temporary for the time being.
> > +            ; How = construct_in_region(_)
> >             ),
> >             MaybeSetArgs = no,
> >             MaybeReuseVar = no
> 
> You need to handle the region_variable in construct_in_region/1
> like reuse vars are handled for the reuse case, i.e. the region variable
> should be part of the non-local set.

The region variable is now added to non-local set.

> 
> > @@ -1129,6 +1131,8 @@
> >         ;
> >             ( How = construct_statically(_)
> >             ; How = construct_dynamically
> > +            % XXX Temporary for the time being.
> > +            ; How = construct_in_region(_)
> >             ),
> >             MaybeSetArgs = no
> >         ),
> 
> Similarly here.

The same handling applied here.

> 
> > Index: rbmm.condition_renaming.m
> > ===================================================================
> > RCS file:
> >
> /home/mercury/mercury1/repository/mercury/compiler/rbmm.condition_renaming.m,v
> > retrieving revision 1.2
> > diff -u -u -r1.2 rbmm.condition_renaming.m
> > --- rbmm.condition_renaming.m	15 Jun 2007 11:46:11 -0000	1.2
> > +++ rbmm.condition_renaming.m	18 Jun 2007 07:46:31 -0000
> 
> ...
> 
> > +:- pred apply_renaming(rpt_graph::in, renaming::in, rptg_node::in,
> > +	set(string)::in, set(string)::out) is det.
> > +
> > +apply_renaming(Graph, Renaming, Node, !Regions) :-
> > +	RegionName = rptg_lookup_region_name(Graph, Node),
> > +	( if	map.search(Renaming, RegionName, RenamedRegionName)
> > +	  then	svset.insert(RenamedRegionName, !Regions)
> > +	  else	svset.insert(RegionName, !Regions)
> > +	).
> 
> The name of that predicate is too general; I would call it something
> like apply_region_renaming.

I changed the name.

> 
> ...
> 
> > Index: rbmm.m
> > ===================================================================
> > RCS file: /home/mercury/mercury1/repository/mercury/compiler/rbmm.m,v
> > retrieving revision 1.4
> > diff -u -u -r1.4 rbmm.m
> > --- rbmm.m	15 Jun 2007 11:46:11 -0000	1.4
> > +++ rbmm.m	18 Jun 2007 07:32:48 -0000
> >
> > +	region_transform(RptaInfoTable, ConstantRTable, DeadRTable, BornRTable,
> > +		ActualRegionArgumentTable, ResurRenamingTable, IteRenamingTable,
> > +		AnnotationTable, ResurRenamingAnnoTable, IteRenamingAnnoTable,
> > +		map.init, _NameToVarTable, !.ModuleInfo, Module),
> > +	write_hlds(2, Module, !IO).
> 
> Why are you calling write_hlds here?  Invoking the compiler with
> --dump-hlds=<rbmm stage #> ought to have the same effect.
> 

It is corrected now.

> > Index: rbmm.region_resurrection_renaming.m
> > ===================================================================
> > RCS file:
> >
>
/home/mercury/mercury1/repository/mercury/compiler/rbmm.region_resurrection_renaming.m,v
> > retrieving revision 1.2
> > diff -u -u -r1.2 rbmm.region_resurrection_renaming.m
> > --- rbmm.region_resurrection_renaming.m	15 Jun 2007 11:46:12 -0000	1.2
> > +++ rbmm.region_resurrection_renaming.m	18 Jun 2007 07:49:45 -0000
> 
> > +            Annotation = NewName ++ " = " ++ RegionName,
> > +            record_annotation(PrevProgPoint, Annotation, !AnnotationProc)
> > +            %true
> >     ).
> 
> Why the commented out bit at the end?
> 

Deleted.

> 
> 
> > Index: rbmm.region_transformation.m
> > ===================================================================
> > RCS file: rbmm.region_transformation.m
> > diff -N rbmm.region_transformation.m
> > --- /dev/null	1 Jan 1970 00:00:00 -0000
> > +++ rbmm.region_transformation.m	18 Jun 2007 06:46:46 -0000
> > @@ -0,0 +1,944 @@
> 
> ...
> 
> > +:- type name_to_prog_var_table == map(pred_proc_id, name_to_prog_var).
> > +:- type name_to_prog_var == map(string, prog_var).
> 
> What are these types used for?  Document them.

Comment added.

> 
> It would be better to use state variables to pass around the
> PredInfo in the above code, e.g.
> 
>  	else
>  		some [!PredInfo] (
>   			module_info_pred_info(!.ModuleInfo, PredId,
>  				!:PredInfo),
>  		...
> 
>  			module_info_set_pred_inof(PredId, !.PredInfo,
>  				!ModuleInfo)
>  		),
>  		!:Processed = [PredId | !.Processed]
> 

I made the suggested change.


> 
> > +:- pred generate_list_of_region_type(int::in, mer_type::in,
> > +	list(mer_type)::out) is det.
> > +
> > +generate_list_of_region_type(N, RegType, RegTypes) :-
> > +	( if	N = 0
> > +	  then
> > +	  	    RegTypes = []
> > +	  else
> > +			generate_list_of_region_type(N - 1, RegType, RegTypes0),
> > +			RegTypes = [RegType | RegTypes0]
> > +	).
> 
> 
> Fix the indentation.

I rechecked the indentation.

> > +
> > +	% Because an atomic goal is turned into a conjunction, we need to
> > +	% flatten its compounding conjunction if it is in one.
> > +	% For switch,
> 
> For switch, ?

Deleted the incomplete and unnecessary any more comment.


> > +name_to_reg_var(Name, RegVar, !NameToVar, !VarSet, !VarTypes) :-
> > +	( if	map.search(!.NameToVar, Name, RegVar0)
> > +	  then
> > +			RegVar = RegVar0
> > +	  else
> > +			varset.new_named_var(!.VarSet, Name, RegVar, !:VarSet),
> 
> Call svvarset.new_named_var/4 threre.

Changed.



> > +
> > +	% Instruction is of the form: "Tx: xxxxxx R..."
> > +	% The first x is the number of the rule, which is not important here.
> > +	% The next 6 x is either "remove" or "create", the last part is the
> > +	% region name. This region name will be subjected to renaming due to
> > +	% if-then-else and region resurrection.
> > +	% This predicate turns such an instruction into a call to a suitable
> > +	% region builtin.
> > +	%
> 
> Instructions should *not* be represented as strings.  You're using
> a strongly-typed language, so use types!  e.g.
> 
>  	:- type region_instruction
>  		--->	region_instruction(
>  				ri_rule_num :: int,
>  				ri_instr_kindg :: region_op,
>  				ri_name :: ???
>  			).
> 
>  	:- type region_op
>  		--->	region_create
>  		;	region_remove.
> 
> or something along those lines.  That would also mean that a lot if
> the if-then-elses that occur in the following code could be turned into
> switches.
> 

I agree to use a type for region instructions (we may have more of them in the
future). What I want to do is to postpone this change a bit after we have a
runnable system. So I put XXX comment at this place. 
Is that acceptable for now?


> ...
> 
> > Index: structure_reuse.indirect.m
> > ===================================================================
> > RCS file:
> >
> /home/mercury/mercury1/repository/mercury/compiler/structure_reuse.indirect.m,v
> > retrieving revision 1.10
> > diff -u -u -r1.10 structure_reuse.indirect.m
> > --- structure_reuse.indirect.m	12 Jun 2007 07:21:26 -0000	1.10
> > +++ structure_reuse.indirect.m	18 Jun 2007 12:50:00 -0000
> > @@ -302,6 +302,8 @@
> >             ;
> >                 ( HowToConstruct = construct_dynamically
> >                 ; HowToConstruct = reuse_cell(_)
> > +                % XXX Temporary for the time being.
> > +                ; HowToConstruct = construct_in_region(_)
> 
> It isn't clear which alternative the XXX comment is supposed to apply
> there.

I removed the unnecessary comment after all.


--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------