This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch] Lno branch merge -- scalar evolutions analyzer



+/* Returns the block at the beginning of the edge. */
+
+static inline struct basic_block_def *
+edge_source (edge e)
+{
+ return e->src;
+}
+
+/* Returns the block at the end of the edge. */
+
+static inline struct basic_block_def *
+edge_destination (edge e)
+{
+ return e->dest;
+}
+


I wonder if we should have a macro here for this "static inline" usage.

When GCC (or another C99-ish compiler) is being used to build the compiler, we probably really want just plain "inline". Or maybe "extern inline"? It's a shame to duplicate this code in all the object files.

Too bad we're not using a language where all the compilers support "inline", like, say, C++. :-)

Anyhow, this has nothing to do with your patch. :-)

Do you think that these accessor functions are worth it? It seems like we already have a ton of code that just does "e->src", etc. It seems to me like we should just use the "->" form, unless we want to go through and change all the existing code. I don't think this is a big deal one way or the other, though.

+/* Returns superloop of LOOP at given DEPTH. */


It's often helpful to use a word different than the one in the name of the function, for maximum clarity. For example:

/* Returns the loop such that LOOP is nested DEPTH (indexed from zero) loops within LOOP. */

+/* Returns the outer loop. */


Similarly, "returns the loop containing LOOP" would be better.

+/* Returns the header basic block of the loop. */


"Returns the basic block through which control flow must proceed to enter the loop. "

And so forth, for the other functions.

Index: tree-chrec.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree-chrec.c,v
retrieving revision 2.1
diff -d -u -p -r2.1 tree-chrec.c
--- tree-chrec.c	30 Jun 2004 15:37:42 -0000	2.1
+++ tree-chrec.c	30 Jun 2004 23:13:58 -0000
@@ -37,47 +37,6 @@ Software Foundation, 59 Temple Place - S
#include "tree-pass.h"


-/* This part will be removed once the merging is finished. */


Why aare you removing all the stuff in tree-chrec.c? Is it no longer needed? If so, should the whole file just go away?

+/* Return the loop of the statement STMT. */
+
+static inline struct loop *
+loop_of_stmt (tree stmt)


I would call this "loop_containing_stmt".

Index: tree-flow.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/tree-flow.h,v
retrieving revision 2.17
diff -d -u -p -r2.17 tree-flow.h
--- tree-flow.h	30 Jun 2004 21:28:59 -0000	2.17
+++ tree-flow.h	30 Jun 2004 23:13:58 -0000
@@ -595,6 +595,27 @@ extern void propagate_value (use_operand
extern void propagate_tree_value (tree *, tree);
extern void replace_exp (use_operand_p, tree);


+/* Description of number of iterations of a loop. */
+struct tree_niter_desc
+{
+ tree assumptions; /* Assumptions for the number of iterations be valid. */
+ tree may_be_zero; /* Condition under that the loop exits in the first
+ iteration. */
+ tree niter; /* Number of iterations. */
+ tree additional_info; /* Additional conditions taken into account when
+ deriving the information above. */
+};


I just reviewed a fair amount of this code in another patch; I thought it was from Zdenek?

That message is here:

http://gcc.gnu.org/ml/gcc-patches/2004-07/msg00184.html

Would you please incorporate those comments?

+ The symbolic chrec representation contains all the semantics of the
+ original code. What is more difficult is to use this information.


Would you please explain the notation you're using in this file?

For example:

b -> {a, +, 1}_1

How does one read that? (I think I understand, having read your paper, but it should be in the comments.)

+/* When CHREC_BEFORE has an evolution part in LOOP_NB, add to this
+ evolution the expression TO_ADD, otherwise construct an evolution
+ part for this loop. */


And return? All functions should say what they return.

+
+static inline tree
+set_nb_iterations_in_loop (struct loop *loop, + tree res)
+{
+ /* After the loop copy headers has transformed the code, each loop
+ runs at least once. */


The grammar here is incorrect; is "loop copy headers" a function name, missing underscores?

+ case POLYNOMIAL_CHREC:


I don't think this should block your patch going in, but do these CHRECs really want to be tree nodes? Could we use a more specific chrec data structure? (We're trying to get away from the "everything is a tree" idea.)

OK with the various documentation changes suggested above as well as the changes in the email message I referred to above.

Thanks,

--
Mark Mitchell
CodeSourcery, LLC
(916) 791-8304
mark@codesourcery.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]