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: [PING^2][PATCH] New pass to partition single function into multiple


Hello Steven,

Thanks for your review. please see my answers below.

> >
> > 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

I intend to fix that in my next posting.

> +  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?

The fields of this structure should be machine specific functions; for
example, SPU back-end implementation for estimate_instruction_size ()
returns the instruction length including the overhead of adding such
instructions in terms of the extra instructions  (i.e. branch hints)
>
>
> +          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.

I will change it.

> -        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.

Here's the snippet which should explain the comment:
(I should move it to before the first line)

                  basic_block bb = NOTE_BASIC_BLOCK (in_rtx);
                  /* 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);

>
>
> +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.

I will do that.
>
>
> +      && cfun
> +      && (crtl->subsections.number_of_sections > 0))
>
> Redundant ( )
>
>
> +      && (flag_partition_functions_into_sections == 0))
>
> Ditto.
>

Will fix that.
>
> +      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?

Please see the comment:

      /* When the function starts with a cold section, we need to
explicitly
         align the hot section and write out the hot section label.
         But if the current function is a thunk, we do not have a CFG.  */
>
>
>
> +/* 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?

I can change that, however it seems that using malloc is also efficient in
this case.
>
> +/* 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?

Yes, the insn size is dynamic.
As was mentioned above, the SPU the machine reorg pass might insert extra
instructions based on some logic which might effect the section size (i.e.
for each branch insert branch hint). In our calculation, the sizes of the
extra instructions will be added to the estimated size of the instruction
which caused their insertion.   After breaking a section the calculation of
the extra instructions for the remaining insns may be changed  (and that's
why the insn size is dynamic).  We based our calculation on the initial
insns size we saved. (this should be good enough for the estimation)
>
>
> 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?
>

I should change that.
>
>
> +     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?

The legal_breakpoint query should prevent us from breaking a critical
section.
A critical section can be any sequence of code that we should not break.
The check_sections function should check whether the maximum section size
was exceeded
by iterating over the final sections and checking their sizes.
>
> 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.

cfg_layout_initialize is called after check_unexpected_insns function.
>
> I stopped reviewing at this point. There is still much to do

Thanks,
Revital
>
> Ciao!
> Steven


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