Re: [PING^2][PATCH] New pass to partition single function into multiple

[Steven Bosscher <stevenb dot gcc at gmail dot com>]

On Tue, May 5, 2009 at 12:35 PM, Revital1 Eres <ERES@il.ibm.com> wrote:
>
> Ping - http://gcc.gnu.org/ml/gcc-patches/2009-04/msg02080.html

I don't see a repost addressing
http://gcc.gnu.org/ml/gcc-patches/2009-04/msg01474.html


+  struct bb_partitioning_into_sections

This has no business in target.h as far as I can tell. Why not put it
in basic-block.h?


+          warning (0, "-fpartition-functions-into-sections disabled; "
+		   " -ffunction-sections not supported for this target");
+          flag_partition_functions_into_sections = 0;

warning (0, ...) is a no-no. OPT_Wmissed_optimization? Maybe even just
sorry(). Likewise elsewhere in the patch. Likewise for the inform()
calls. If the option is not supported then IMHO GCC should just bail
out.


-		  if (bb != 0)
+                  /* When partitioning into multiple sections
+                     NOTE_BASIC_BLOCK holds the section id.  */
+		  if (bb != 0 && !flag_partition_functions_into_sections)
 		    fprintf (outfile, " [bb %d]", bb->index);

As they'd say in Germany: "Ja, und?"
What does this comment mean, I don't see NOTE_BASIC_BLOCK be used. I
also see no reason to _not_ print the basic block ID if
flag_partition_functions_into_sections is set.


+fpartition-functions-into-sections
+Common Report Var(flag_partition_functions_into_sections)
+Partition function into sections
+
+fpartition-functions-into-sections=
+Common RejectNegative Joined UInteger
+-fpartition-functions-into-sections=<number>
+Partition function into sections.  Number indicates the maximum number
+of bytes each section can contain

Add "Optimization" to both options.


+      && cfun
+      && (crtl->subsections.number_of_sections > 0))

Redundant ( )


+      && (flag_partition_functions_into_sections == 0))

Ditto.


+      if (!cfun->is_thunk
+          && BB_PARTITION (ENTRY_BLOCK_PTR->next_bb) == BB_COLD_PARTITION)
+        {
+          first_function_block_is_cold = true;
+        }

Why are thunks treated as special?



+/* The current size (in elements) of the following dynamic array.  */
+static unsigned HOST_WIDE_INT fbb_data_size = 0;
+
+/* The array which holds needed information for basic blocks.  */
+static funcpart_basic_block_data *fbb_data;

Why not use a VEC here?


+/* Information regarding insn.  */
+struct insn_aux
+{
+  /* The insn.  */
+  rtx insn;
+
+  /* The estimated size in bytes of insn.  */
+  unsigned HOST_WIDE_INT size;
+};

Is the insn size dynamic somehow?  Otherwise you can just use
get_attr_length(). Instead, you're caching get_attr_min_length().
Are you optimistically assume insns will have their minimum length?
Doesn't that conflict with the splitting of the function based on a
maximum size per section?
Is the overhead of these get_attr_*length functions so much that you
need to cache the result?


In estimate_size_of_insns_in_bb() you calculate the size of a bb, and
then check it against the previously calculated size if the size was
already calculated before. Why?  Why not just use the previously
calculated size? If you really want to check, do it the other way
around: assume the previously calculated size is OK, and check that
with ENABLE_CHECKING. But the whole checking seems kind of silly to
me. Is this code error-prone?



+	  if (((targetm.bb_partitioning.legal_breakpoint != 0)
+	       && targetm.bb_partitioning.legal_breakpoint (insn))
+	      || (targetm.bb_partitioning.legal_breakpoint == 0))

Too many parentheses. Unclear code, too. What happens if the break
point is not legal? You somehow allow the block to exceed the maximum
size?  How is this tested?

General remark: gross lack of test cases.


+check_unexpected_insns (rtx rtx_first)

Does not work in cfglayout mode. You will not find insns between basic
blocks in cfglayout mode.  You have to look for things like tablejump
data in the basic block header/footer.

I stopped reviewing at this point. There is still much to do

Ciao!
Steven