On Wed, Jan 4, 2017 at 8:08 PM Alexandre Oliva <aol...@redhat.com> wrote:
>
> On Jan 4, 2017, Alexandre Oliva <aol...@redhat.com> wrote:
>
> > So I guess we need some alternate PerFunction option flag that makes
> > it per-function, but not part of the ICF hash?
>
> Like this...
>
> If we include them in the ICF hash, they may cause congruence_groups to
> be processed in a different order due to different hashes, which in turn
> causes different funcdef_nos to be assigned to functions. Since these
> numbers are included in -fcompare-debug dumps, they cause failures.
>
> Since these options are not optimization options, in that they do not
> (or should not, save for bugs like this) affect the executable code
> output by the compiler, they should not be marked as such.
>
> This patch replaces the Optimization flag in the var-tracking options
> with the newly-introduced PerFunction flag, so that it can still be
> controlled on a per-function basis, but that disregards it in the hash
> computation used by ICF.
>
> This fixes -fcompare-debug failures in numerous LTO testcases.
>
> Regstrapped on x86_64-linux-gnu and i686-linux-gnu. Ok to install?
>
> for gcc/ChangeLog
>
> * doc/options.texi (PerFunction): New.
> * opt-functions.awk (switch_flags): Map both Optimization and
> PerFunction to CL_OPTIMIZATION.
> * opth-gen.awk: Test for PerFunction flag along with
> Optimization.
> * optc-save-gen.awk: Likewise. Introduce var_opt_hash and set
> it only when the latter is present. Skip those that don't in
> the hash function generator.
> * common.opt (fvar-tracking): Mark as PerFunction instead of
> Optimization.
> (fvar-tracking-assignments): Likewise.
> (fvar-tracking-assignments-toggle): Likewise.
> (fvar-tracking-uninit): Likewise.
> diff --git a/gcc/optc-save-gen.awk b/gcc/optc-save-gen.awk
> index 8ce4248..0928d5d 100644
> --- a/gcc/optc-save-gen.awk
> +++ b/gcc/optc-save-gen.awk
> @@ -100,7 +100,7 @@ var_opt_range["optimize_debug"] = "0, 1";
> # cache.
>
> for (i = 0; i < n_opts; i++) {
> - if (flag_set_p("Optimization", flags[i])) {
> + if (flag_set_p("(Optimization|PerFunction)", flags[i])) {
> name = var_name(flags[i])
> if(name == "")
> continue;
> @@ -743,7 +743,7 @@ var_opt_val[2] = "x_optimize_debug"
> var_opt_val_type[1] = "char "
> var_opt_val_type[2] = "char "
> for (i = 0; i < n_opts; i++) {
> - if (flag_set_p("Optimization", flags[i])) {
> + if (flag_set_p("(Optimization|PerFunction)", flags[i])) {
> name = var_name(flags[i])
> if(name == "")
> continue;
> @@ -756,6 +756,7 @@ for (i = 0; i < n_opts; i++) {
> otype = var_type_struct(flags[i])
> var_opt_val_type[n_opt_val] = otype;
> var_opt_val[n_opt_val++] = "x_" name;
> + var_opt_hash[n_opt_val] = flag_set_p("Optimization",
> flags[i]);
> }
> }
> print "";
This patch was committed a couple of years ago, but it's buggy in two
ways. The var_opt_hash array is not set for the three flags that
precede this loop (x_optimize, x_optimize_size, and x_optimize_debug).
And, as you can see by noticing the position of n_opt_val++ above,
it's set for the wrong flag for the other cases--it's always set for
the next flag, not the current one. This causes the hash and equality
comparisons of cl_optimization_eq to do the wrong thing. It's hard to
notice, because in practice there tend to be other changes that affect
the result. I was able to construct a (rather fragile) test case that
shows the problem, in which the compiler thinks that two sets of
function optimization attributes are the same when using the hash
table in build_optimization_node, although one is -O2 and one is -Os.
Any objections to my committing this patch? Bootstrapped and tested
on x86_64-pc-linux-gnu.
Ian
2019-02-13 Ian Lance Taylor <i...@golang.org>
* optc-save-gen.awk: Set var_opt_hash for initial optimizations
and set current index for other optimizations.
2019-02-13 Ian Lance Taylor <i...@golang.org>
* gcc.dg/func-attr-1.c: New test.
Index: optc-save-gen.awk
===================================================================
--- optc-save-gen.awk (revision 268833)
+++ optc-save-gen.awk (working copy)
@@ -770,8 +770,11 @@
n_opt_val = 3;
var_opt_val[0] = "x_optimize"
var_opt_val_type[0] = "char "
+var_opt_hash[0] = 1;
var_opt_val[1] = "x_optimize_size"
+var_opt_hash[1] = 1;
var_opt_val[2] = "x_optimize_debug"
+var_opt_hash[2] = 1;
var_opt_val_type[1] = "char "
var_opt_val_type[2] = "char "
for (i = 0; i < n_opts; i++) {
@@ -787,8 +790,9 @@
otype = var_type_struct(flags[i])
var_opt_val_type[n_opt_val] = otype;
- var_opt_val[n_opt_val++] = "x_" name;
+ var_opt_val[n_opt_val] = "x_" name;
var_opt_hash[n_opt_val] = flag_set_p("Optimization", flags[i]);
+ n_opt_val++;
}
}
print "";
Index: testsuite/gcc.dg/func-attr-1.c
===================================================================
--- testsuite/gcc.dg/func-attr-1.c (nonexistent)
+++ testsuite/gcc.dg/func-attr-1.c (working copy)
@@ -0,0 +1,23 @@
+/* Test that setting -Os in a function attribute works. */
+/* { dg-do compile } */
+/* { dg-options "-O2 -fdump-rtl-bbro" } */
+
+extern void f1 ();
+extern int f2 ();
+
+__attribute__ ((__optimize__ ("O2")))
+void
+f3()
+{
+}
+
+__attribute__ ((__optimize__ ("-Os", "-falign-functions", "-falign-jumps",
"-falign-labels", "-falign-loops", "-fno-inline-functions",
"-foptimize-strlen")))
+int
+f4 () {
+ if (f2 () == 0) {
+ f1 ();
+ }
+ return 0;
+}
+
+/* { dg-final { scan-rtl-dump-not "Duplicated bb" "bbro" } } */
