This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: cfg merge part 18 - superblock formation pass
- From: Richard Henderson <rth at redhat dot com>
- To: Jan Hubicka <jh at suse dot cz>
- Cc: gcc-patches at gcc dot gnu dot org, gcc-pdo at atrey dot karlin dot mff dot cuni dot cz, aj at suse dot de
- Date: Fri, 31 May 2002 16:00:52 -0700
- Subject: Re: cfg merge part 18 - superblock formation pass
- References: <20020524134635.GD10556@atrey.karlin.mff.cuni.cz>
On Fri, May 24, 2002 at 03:46:35PM +0200, Jan Hubicka wrote:
> * Makefile.in (tracer.o): New.
> * params.def (TRACER_*): New options.
> * rtl.h (tracer): Declare.
> * timevar.def (TV_TRACER): New.
> * toplev.c (dump_file_index): Add DFI_tracer.
> (dump_file_info): Add tracer.
> (flag_tracer): New.
> (lang_indepdenent_options): Add tracer.
> (rest_of_compilation): Call tracer.
> * tracer.c: New file.
> * invoke.texi (-ftracer): Document.
> (--param tracer-*): Document.
Looks generally good.
s/htan/than/g
Please re-wrap at 80 columns. There's quite a few lines
in tracer.c that are too long.
> + "How large fraction should be covered by trace formation weighted by the execution frequencies (in percents). Used when profile feedback is available",
The percentage of the function, weighted by execution frequency,
that must be covered in order to be considered for trace
formation.
> + tracer ();
> + cleanup_cfg (CLEANUP_EXPENSIVE);
> + #ifdef ENABLE_CHECKING
> + verify_flow_info ();
> + #endif
Seems like the verify_flow_info belongs inside tracer.
> + if (best->probability <= ((int) (REG_BR_PROB_BASE / 100
> + * (profile_info.count_profiles_merged
> + && flag_branch_probabilities
> + ? PARAM_VALUE
> + (TRACER_MIN_BRANCH_PROBABILITY_FEEDBACK)
> + : PARAM_VALUE
> + (TRACER_MIN_BRANCH_PROBABILITY)))))
I think this would be clearer as
if (...)
probability_cutoff = PARM_VALUE (...)
else
probability_cutoff = PARM_VALUE (...)
probability_cutoff = REG_BR_PROB_BASE * probability_cutoff / 100;
if (best_probability <= probability_cutoff)
> + if (EDGE_FREQUENCY (best) * REG_BR_PROB_BASE
> + < bb->frequency * (int) (REG_BR_PROB_BASE / 100
> + * PARAM_VALUE (TRACER_MIN_BRANCH_RATIO)))
Likewise.
> + if (find_best_successor (bb2) != e
> + || seen (bb2)
> + || (e->flags & (EDGE_DFS_BACK | EDGE_COMPLEX)))
A better ordering here would be find_best_successor last.
> + if (find_best_predecessor (bb) != e
> + || seen (bb)
> + || (e->flags & (EDGE_DFS_BACK | EDGE_COMPLEX)))
Likewise.
> + for (i = 0; i < n_basic_blocks; i++)
Naturally this will now have to be updated...
> + int n = count_insns (BASIC_BLOCK (i));
Seems like you should be caching these, since you compute this
number again later. We'd rather not scan the entire insn list
twice, I think.
> + if (profile_info.count_profiles_merged
> + && flag_branch_probabilities)
> + cover_insns = ((weighted_insns
> + * PARAM_VALUE (TRACER_DYNAMIC_COVERAGE_FEEDBACK) + 50) / 100);
> + else
> + cover_insns = ((weighted_insns * PARAM_VALUE (TRACER_DYNAMIC_COVERAGE) + 50)
> + / 100);
Again, clearer if the parm value pulled out inside the if
and the common computation done after the if.
> + for (; traced_insns < cover_insns
> + && nduplicated < max_dup_insns
> + && !fibheap_empty (heap);)
while.
> + Perform tail duplication to enlarge superblock size. These transformations
> + simplify control structure of programs allowing other optimizations to do
This transformation simplifies the control structure of the function, ...
> + @item tracer-dynamic-coverage
> + @item tracer-dynamic-coverage-feedback
itemx. several more of these.
r~