[Mono-dev] [PATCH] Tree mover, again

[Paolo Molaro]

> Index: mono/mono/mini/local-propagation.c
> ===================================================================
> --- mono/mono/mini/local-propagation.c	(revision 0)
> +++ mono/mono/mini/local-propagation.c	(revision 0)
> @@ -0,0 +1,1117 @@
> +/*
> + * local-propagation.c: Local constant, copy and tree propagation.
> + *
> + * Author:
> + *   Massimiliano Mantione (massi at ximian.com)

Since some of the code was copied from mini.c, copy over also the
copyright/author notice (Dietmar wrote most of it, IIRC).

> +extern MonoJitICallInfo **emul_opcode_map;
> +static inline MonoJitICallInfo *
> +find_jit_opcode_emulation (int opcode)
> +{
> +	if  (emul_opcode_map)
> +		return emul_opcode_map [opcode];
> +	else
> +		return NULL;
> +}

Not acceptable, export the existing mono_find_jit_opcode_emulation ()
instead.

> +#define MONO_DEBUG_LOCAL_PROP 0
> +#define MONO_DEBUG_TREE_MOVER 0
> +#define MONO_DUMP_TREE_MOVER 0
> +#define MONO_APPLY_TREE_MOVER_TO_SINGLE_METHOD 0
> +#define MONO_APPLY_TREE_MOVER_TO_COUNTED_METHODS 0
> +
> +struct MonoTreeMoverActSlot;
> +typedef struct MonoTreeMoverDependencyNode {
> +	struct MonoTreeMoverActSlot *used_slot;
> +	struct MonoTreeMoverActSlot *affected_slot;
> +	struct MonoTreeMoverDependencyNode *next_used_local;
> +	struct MonoTreeMoverDependencyNode *next_affected_local;
> +	struct MonoTreeMoverDependencyNode *previous_affected_local;
> +	gboolean use_is_direct;
> +} MonoTreeMoverDependencyNode;

I would %s/MonoTree/Tree/g, the names are unnecessarily long and they
are not user-visible so the Mono prefix can be removed.

> +#define TREE_MOVER_NEW_NODE(__node) do {\

Used in one place only, remove.

> +#define TREE_MOVER_NEW_MOVE(__move) do {\

Used in one place only, remove.

> +#define TREE_MOVER_DISPOSE_MOVE(__move) do {\

Same.

> +#define TREE_MOVER_DISPOSE_USED_NODES do {\

Unused.

> +#define TREE_MOVER_LINK_AFFECTING_NODE(__node,__affected_slot) do {\

Used once...

> +#define TREE_MOVER_CLEAR_FORWARDING_DEPENDENCY(__slot) do {\

Used once...

> +#define TREE_MOVER_ENFORCE_FORWARDING_DEPENDENCY(__slot) do {\

Same.

> +#define TREE_MOVER_CLEAN_ACT_SLOT_DEPENDENCY_NODES(__slot) do {\
[...]
> +#define TREE_MOVER_CLEAN_ACT_SLOT_PENDING_MOVE(__slot) do {\

Idem.

I stopped reviewing macros at this point. Are you sure you need 30
macros some of which are unused and some of which are used just once?
This code needs serious cleanup to make it readable.

> +static gboolean stind_needs_conversion[(CEE_STIND_R8-CEE_STIND_REF)+1][STACK_MAX] = {

These arrays need to be declared const and use guchar instead of
gboolean to avoid wasting memory.

> +static MonoTreeMoverTreeMove*
> +mono_cprop_copy_values (MonoCompile *cfg, MonoTreeMover *tree_mover, MonoInst *tree, MonoInst **acp)
> +{
> +	MonoInst *cp;
> +	int arity;
> +	MonoTreeMoverTreeMove *pending_move = NULL;
> +
> +	if (tree->ssa_op == MONO_SSA_LOAD && (tree->inst_i0->opcode == OP_LOCAL || tree->inst_i0->opcode == OP_ARG) &&
> +	    (cp = acp [tree->inst_i0->inst_c0]) && !tree->inst_i0->flags) {

Conditionals that don't fit in one line need to be indented by two TABs,
no spaces.

> +
> +		if (cp->opcode == OP_ICONST) {
> +			if (cfg->opt & MONO_OPT_CONSPROP) {
> +				//{ static int c = 0; printf ("CCOPY %d %d %s\n", c++, cp->inst_c0, mono_method_full_name (cfg->method, TRUE)); }
> +#if MONO_DEBUG_LOCAL_PROP
> +				printf ("Propagating constant, tree ");
> +				mono_print_tree (tree);
> +				printf (" becomes ");
> +				mono_print_tree (cp);
> +				printf ("\n");
> +#endif
> +				*tree = *cp;

There are too many preprocessor conditionals in the code: please change
it to not use them. In most cases you can use normal if statements, at
least the code would align properly, like:

				if (MONO_DEBUG_LOCAL_PROP) {
					...
				}

But please note that in the first 45 lines of code there are only 8
lines of non-debugging stuff. It is unreadable, please cleanup.

> +			} else if ((i1->type==STACK_I8)||(i1->opcode==OP_I8CONST)||(i1->opcode==OP_R4CONST)||(i1->opcode==OP_R8CONST)||(i1->opcode==OP_AOTCONST)) {

Use spaces around operators as per the coding style.

> +void
> +mono_local_cprop (MonoCompile *cfg) {
> +	MonoBasicBlock *bb;
> +	MonoInst **acp;
> +	MonoTreeMover *tree_mover;
> +
> +	acp = alloca (sizeof (MonoInst *) * cfg->num_varinfo);

Just us malloc here, this can become too large.

> Index: mono/mono/mini/mini.h
> ===================================================================
> --- mono/mono/mini/mini.h	(revision 57888)
> +++ mono/mono/mini/mini.h	(working copy)
[...]
> @@ -615,6 +616,7 @@
>  #ifdef __ia64
>  	guint8           ins, locals, outs; /* reg stack region sizes */
>  #endif /* __ia64 */
> +	int		num_calls;

This is unused.

Once the code will be cleaned up, please commit.
Thanks.

lupus

-- 
-----------------------------------------------------------------
lupus at debian.org                                     debian/rules
lupus at ximian.com                             Monkeys do it better