[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

Martin Liška  changed:

   What|Removed |Added

  Known to work||8.0
  Known to fail|8.0 |

--- Comment #25 from Martin Liška  ---
Fixed on trunk so far.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #24 from Martin Liška  ---
Author: marxin
Date: Tue Mar 13 08:20:27 2018
New Revision: 258480

URL: https://gcc.gnu.org/viewcvs?rev=258480=gcc=rev
Log:
Fix PTA info in IPA ICF (PR ipa/84658).

2018-03-13  Martin Liska  

PR ipa/84658.
* (sem_item_optimizer::sem_item_optimizer): Initialize new
vector.
(sem_item_optimizer::~sem_item_optimizer): Release it.
(sem_item_optimizer::merge_classes): Register variable aliases.
(sem_item_optimizer::fixup_pt_set): New function.
(sem_item_optimizer::fixup_points_to_sets): Likewise.
* ipa-icf.h: Declare new variables and functions.
2018-03-13  Martin Liska  

PR ipa/84658.
* g++.dg/ipa/pr84658.C: New test.

Added:
trunk/gcc/testsuite/g++.dg/ipa/pr84658.C
Modified:
trunk/gcc/ChangeLog
trunk/gcc/ipa-icf.c
trunk/gcc/ipa-icf.h
trunk/gcc/testsuite/ChangeLog

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #23 from rguenther at suse dot de  ---
On Thu, 8 Mar 2018, marxin at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658
> 
> --- Comment #22 from Martin Liška  ---
> (In reply to Martin Liška from comment #21)
> > (In reply to Jakub Jelinek from comment #20)
> > > How would IPA-PTA work then?
> > 
> > Not having experience how that works, but I would expect a complete rebuild.
> > Similar to what the tree PTA does?
> 
> Note that the TREE PTA is just a TODO_rebuild_alias flag. Thus I would expect
> IPA PTA is also a rebuild of PTA information.

IPA PTA rebuilds everything (and with -fipa-pta we should be fine).  The 
issue is the time between IPA ICF and the late pass_build_alias,
we do the "invalid" folding in pass_ccp.

And yes, we do want to do some scalar cleanup before running PTA.

If we'd decide not to then the "proper" place would probably be
pass_ipa_pta which then either would do local PTA or IPA PTA based
on the flag.

But that's too much for GCC 8 or 7.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #22 from Martin Liška  ---
(In reply to Martin Liška from comment #21)
> (In reply to Jakub Jelinek from comment #20)
> > How would IPA-PTA work then?
> 
> Not having experience how that works, but I would expect a complete rebuild.
> Similar to what the tree PTA does?

Note that the TREE PTA is just a TODO_rebuild_alias flag. Thus I would expect
IPA PTA is also a rebuild of PTA information.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #21 from Martin Liška  ---
(In reply to Jakub Jelinek from comment #20)
> How would IPA-PTA work then?

Not having experience how that works, but I would expect a complete rebuild.
Similar to what the tree PTA does?

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #20 from Jakub Jelinek  ---
How would IPA-PTA work then?

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #19 from Martin Liška  ---
Just having chat with Honza and he is curious why IPA passes should maintain
points-to-sets info as it's rebuild in pass_build_alias quite soon in post IPA
passes? Another question is why the pass_rebuild_alias isn't the first one and
is sitting after CCP?

Isn't also possible to release all points-to-sets before IPA passes? That can
save some memory.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #18 from rguenther at suse dot de  ---
On Thu, 8 Mar 2018, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658
> 
> --- Comment #17 from Jakub Jelinek  ---
> (In reply to Richard Biener from comment #16)
> > Hum, isn't the bug INVALID then?  Shouldn't we restrict 
> > -fmerge-all-constants
> 
> No.
> 
> > to not address-taken variables to not make the option useless?
> 
> -fmerge-all-constants is an extension where the addresses of the constant
> variables may be the same even if without the option they would need to be
> guaranteed to be different.  That is the whole point of the option, many 
> people
> just don't care about it and care more about .rodata size, so they have an
> option to tell the compiler about it.

That's not really an answer to the question ...

But anyway, we have precedence in points-to-set editing which we do
during RTL expansion to reflect variable coalescing.

So ... a different approach for fixing would be if ICF would keep

 a) a global bitmap of all DECL_UID that took part in any merging operation
 b) a mapping of merged DECL_UID to prevailing DECL_UID (which is also the
DECL_PT_UID of the merged ones)

then during IPA ICF transform phase do

  FOR_EACH_DEFINED_FUNCTION (...)
{
   FOR_EACH_SSA_NAME_FN (..., name)
 if (POINTER_TYPE_P (TREE_TYPE (name))
 && SSA_NAME_PTR_INFO (name))
   fixup_pt_set (_NAME_PTR_INFO (name)->pt);
   fixup_pt_set (fn->gimple_df->escaped);

   /* The above get's us to 99% I guess, at least catching the
  address compares.  Below also gets us aliasing correct
  but as said we're giving leeway to the situation with
  readonly vars anyway, so ... */
   FOR_EACH_BB_FN (...)
 for (each-stmt)
   if (is_gimple_call (...))
 {
   fixup_pt_set (gimple_call_use_set (call));
   fixup_pt_set (gimple_call_clobber_set (call));
 }
}

and fixup_pt_set essentially doing

  EXECUTE_IF_AND_IN_BITMAP (pt->vars, bitmap-of-merged-decls, 0, i, bi)
bitmap_set_bit (pt->vars, *merged_uid_to_pt_uid->get (i));

that would be a "real" fix, also allowing the forthcoming pointer
comparison optimization using two pointers (and thus two points-to
sets).

> > I presume -fmerge-all-constants pre-dates ICF?
> 
> Yes, by far.  -fmerge-all-constants is from 2001, ICF from 2014.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #17 from Jakub Jelinek  ---
(In reply to Richard Biener from comment #16)
> Hum, isn't the bug INVALID then?  Shouldn't we restrict -fmerge-all-constants

No.

> to not address-taken variables to not make the option useless?

-fmerge-all-constants is an extension where the addresses of the constant
variables may be the same even if without the option they would need to be
guaranteed to be different.  That is the whole point of the option, many people
just don't care about it and care more about .rodata size, so they have an
option to tell the compiler about it.

> I presume -fmerge-all-constants pre-dates ICF?

Yes, by far.  -fmerge-all-constants is from 2001, ICF from 2014.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #16 from Richard Biener  ---
(In reply to Martin Liška from comment #14)
> (In reply to Richard Biener from comment #13)
> > So you can loses the TREE_ADDRESSABLE restriction somewhat in requiring at
> > most one decl to be TREE_ADDRESSABLE - that's the one you need to keep, the
> > others may become aliases.  Given TREE_ADDRESSABLE isn't reliable for
> > !TREE_STATIC vars
> > we can't merge any exported decls that way.  Consider
> > 
> > const int foo1;
> > const int foo2;
> > 
> > int *bar();  // in other TU, returns address of foo1
> > 
> > int main ()
> > {
> >   if (bar() != )
> > ...;
> > }
> > 
> > not sure if we want to do hand-waving saying that we can't possibly have
> > points-to sets mentioning those without seeing the function IL.  Well,
> > might be similarly hand-waving as the reference aliasing issue.
> > 
> > So we could drop the TREE_STATIC restriction if TREE_ADDRESSABLE is reliable
> > within the current TU.
> 
> Note that this issue happens only with -fmerge-all-constants. And we have
> caveat
> in documentation that using the option, pointer comparison can be broken:
> 
> Languages like C or C++ require each variable,
>including multiple instances of the same variable in recursive
> calls, to have distinct locations, so using this option results in
> non-conforming behavior.
> 
> Thus:
> 
> $ cat test.c
> const int foo1 = 3;
> const int foo2 = 3;
> 
> const int *
> __attribute__((noinline))
> bar()
> {
>   return 
> }
> 
> int main ()
> {
>   if ((bar() - ) == 0)
> __builtin_abort ();
> 
>   return 0;
> }
> 
> $ gcc test.c -O2 -fno-ipa-icf -fmerge-all-constants  && ./a.out 
> Aborted (core dumped)
> 
> That said, would be Richi your comment in c#12 a sufficient fix?

Hum, isn't the bug INVALID then?  Shouldn't we restrict -fmerge-all-constants
to not address-taken variables to not make the option useless?

The suggested fix in c#12 is a band-aid only.

What's the impact of not merging TREE_ADDRESSABLE decls into other decls?

I presume -fmerge-all-constants pre-dates ICF?

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #15 from Jakub Jelinek  ---
(In reply to Martin Liška from comment #14)
> (In reply to Richard Biener from comment #13)
> > So you can loses the TREE_ADDRESSABLE restriction somewhat in requiring at
> > most one decl to be TREE_ADDRESSABLE - that's the one you need to keep, the
> > others may become aliases.  Given TREE_ADDRESSABLE isn't reliable for
> > !TREE_STATIC vars
> > we can't merge any exported decls that way.  Consider
> > 
> > const int foo1;
> > const int foo2;
> > 
> > int *bar();  // in other TU, returns address of foo1
> > 
> > int main ()
> > {
> >   if (bar() != )
> > ...;
> > }
> > 
> > not sure if we want to do hand-waving saying that we can't possibly have
> > points-to sets mentioning those without seeing the function IL.  Well,
> > might be similarly hand-waving as the reference aliasing issue.
> > 
> > So we could drop the TREE_STATIC restriction if TREE_ADDRESSABLE is reliable
> > within the current TU.
> 
> Note that this issue happens only with -fmerge-all-constants. And we have
> caveat
> in documentation that using the option, pointer comparison can be broken:
> 
> Languages like C or C++ require each variable,
>including multiple instances of the same variable in recursive
> calls, to have distinct locations, so using this option results in
> non-conforming behavior.
> 
> Thus:
> 
> $ cat test.c
> const int foo1 = 3;
> const int foo2 = 3;
> 
> const int *
> __attribute__((noinline))
> bar()
> {
>   return 
> }
> 
> int main ()
> {
>   if ((bar() - ) == 0)
> __builtin_abort ();
> 
>   return 0;
> }
> 
> $ gcc test.c -O2 -fno-ipa-icf -fmerge-all-constants  && ./a.out 
> Aborted (core dumped)

Sure, the caveat of -fmerge-all-constants is that different constants can be
merged.
But that doesn't imply that the #c0 failure is fine, the testcase doesn't
really care if kTestCases constant arrays from the two functions are merged
together or not, if not, they will be separate, if yes, they will be the same,
but if it doesn't try to compare them in any way, it should make zero
difference on the behavior.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #14 from Martin Liška  ---
(In reply to Richard Biener from comment #13)
> So you can loses the TREE_ADDRESSABLE restriction somewhat in requiring at
> most one decl to be TREE_ADDRESSABLE - that's the one you need to keep, the
> others may become aliases.  Given TREE_ADDRESSABLE isn't reliable for
> !TREE_STATIC vars
> we can't merge any exported decls that way.  Consider
> 
> const int foo1;
> const int foo2;
> 
> int *bar();  // in other TU, returns address of foo1
> 
> int main ()
> {
>   if (bar() != )
> ...;
> }
> 
> not sure if we want to do hand-waving saying that we can't possibly have
> points-to sets mentioning those without seeing the function IL.  Well,
> might be similarly hand-waving as the reference aliasing issue.
> 
> So we could drop the TREE_STATIC restriction if TREE_ADDRESSABLE is reliable
> within the current TU.

Note that this issue happens only with -fmerge-all-constants. And we have
caveat
in documentation that using the option, pointer comparison can be broken:

Languages like C or C++ require each variable,
   including multiple instances of the same variable in recursive
calls, to have distinct locations, so using this option results in
non-conforming behavior.

Thus:

$ cat test.c
const int foo1 = 3;
const int foo2 = 3;

const int *
__attribute__((noinline))
bar()
{
  return 
}

int main ()
{
  if ((bar() - ) == 0)
__builtin_abort ();

  return 0;
}

$ gcc test.c -O2 -fno-ipa-icf -fmerge-all-constants  && ./a.out 
Aborted (core dumped)

That said, would be Richi your comment in c#12 a sufficient fix?

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #13 from Richard Biener  ---
So you can loses the TREE_ADDRESSABLE restriction somewhat in requiring at most
one decl to be TREE_ADDRESSABLE - that's the one you need to keep, the others
may become aliases.  Given TREE_ADDRESSABLE isn't reliable for !TREE_STATIC
vars
we can't merge any exported decls that way.  Consider

const int foo1;
const int foo2;

int *bar();  // in other TU, returns address of foo1

int main ()
{
  if (bar() != )
...;
}

not sure if we want to do hand-waving saying that we can't possibly have
points-to sets mentioning those without seeing the function IL.  Well,
might be similarly hand-waving as the reference aliasing issue.

So we could drop the TREE_STATIC restriction if TREE_ADDRESSABLE is reliable
within the current TU.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #12 from Richard Biener  ---
(In reply to Martin Liška from comment #11)
> (In reply to Jakub Jelinek from comment #10)
> > If ICF needs to adjust all points-to if it makes any variable aliases,
> > perhaps it should as well adjust the code to use the variables rather than
> > their aliases.
> 
> Similar to what we do in sanopt.c:rewrite_usage_of_param? If so, I can do
> that.

But that doesn't help - you'll have points-to sets mentioning both DECLs.

static decls without TREE_ADDRESSABLE set are fine to merge, those won't
ever appear in points-to sets.  For all others you really need to adjust
points-to sets.

Note the issue is "new" since we optimize address comparisons since the
idea was that for memory references aliasing doesn't really matter since
all decls we merge are readonly(?).  That's of course a bit of a fishy
view...

Given ptrs_compare_unequal is as restricted as it is now we could work
around this issue there by doing instead of

  if (VAR_P (obj1)
  && (TREE_STATIC (obj1) || DECL_EXTERNAL (obj1)))
{
  varpool_node *node = varpool_node::get (obj1);
  /* If obj1 may bind to NULL give up (see below).  */
  if (! node
  || ! node->nonzero_address ()
  || ! decl_binds_to_current_def_p (obj1))
return false;
}

sth like

  if (VAR_P (obj1)
  && (TREE_STATIC (obj1) || DECL_EXTERNAL (obj1)))
{
  varpool_node *node = varpool_node::get (obj1);
  /* If obj1 may bind to NULL give up (see below).  */
  if (! node
  || ! node->nonzero_address ()
  || ! decl_binds_to_current_def_p (obj1))
return false;
  FOR_EACH_ALIAS (node, ...)
{
  tree obj1_alias = ...;
  unsigned saved_pt_uid = DECL_PT_UID (obj1_alias);
  DECL_PT_UID (obj1_alias) = DECL_UID (obj1_alias);
  bool aliases = pt_solution_includes (>pt, obj1_alias);
  DECL_PT_UID (obj_alias) = saved_pt_uid;
}
 }

with the code walking over aliases adjusted accordingly.

Of course if we ever try to compare two pointers and thus have just
two points-to sets to intersect we're still lost.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #11 from Martin Liška  ---
(In reply to Jakub Jelinek from comment #10)
> If ICF needs to adjust all points-to if it makes any variable aliases,
> perhaps it should as well adjust the code to use the variables rather than
> their aliases.

Similar to what we do in sanopt.c:rewrite_usage_of_param? If so, I can do that.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #10 from Jakub Jelinek  ---
If ICF needs to adjust all points-to if it makes any variable aliases, perhaps
it should as well adjust the code to use the variables rather than their
aliases.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

Richard Biener  changed:

   What|Removed |Added

   Keywords||alias
 Status|NEW |ASSIGNED

--- Comment #9 from Richard Biener  ---
This is a points-to issue.  After inlining we have

  static const intD.9 aD.2281[16] = {0, 1, 2, 3, 4, 5, 8, 15, 16, 17, 512,
1020, 1021, 1022, 1023, 1024};
  intD.9 bD.2313;
  const intD.9 * __for_beginD.2314;
  static const intD.9 aD.2291ptD.2281[16];
...
   [100.00%]:
  # PT = { D.2281 } (nonlocal)
  # ALIGN = 4, MISALIGN = 0
  # __for_begin_8 = PHI <(2), __for_begin_10(4)>
  if (__for_begin_8 == [(voidD.43 *) + 64B])
goto ; [5.88%]

good copy!

   [100.00%]:
  # PT = { D.2291 } (nonlocal)
  # ALIGN = 4, MISALIGN = 0
  # __for_begin_5 = PHI <(5), __for_begin_7(7)>
  if (__for_begin_5 == [(voidD.43 *) + 64B])
goto ; [5.88%]
  else
goto ; [94.12%]

bad copy!  See how __for_begin_5 only points to D.2291 -- remember the
points-to sets are just bits.  But the DECL we check against has
been adjusted to the DECL_PT_UID of 2281...

I guess this is finally a case where we wondered if we get things correct... :/
I remember saying you need to adjust all existing points-to sets

Note for the function bodies after inlining I see foo () has points-to
retained but bar () has it seemingly cleared?  But maybe this just dump
before applying the IPA ICF transform.

The ICF dump says:

Unified; Variable alias has been created.
  Setting points-to UID of [a.2291] as 2281

but then existing points-to sets containing 2291 are not adjusted.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #8 from Jakub Jelinek  ---
(In reply to Martin Liška from comment #6)
> (In reply to Jakub Jelinek from comment #3)
> > Seems the inliner immediately undoes what ICF did and both get inlined into
> > main as well.
> 
> It's not undoing the decision because the symbol (Bar) is global. So it both
> inlines into main and the wrapper is preserved. But it's not triggering the
> issue.

Well, it does, because it inlines Foo back into Bar, so all the ICF effort
except for creating the variable alias is gone.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #7 from Jakub Jelinek  ---
Tried to look at the ccp2-uid-details dump and can't make any sense from that,
so I think we need Richi on this.

A guess would be that something somewhere looks through the alais at one point
and not the other point, it is referenced just in
  # __for_begin_5 = PHI <(5), __for_begin_7(7)>
  if (__for_begin_5 == [(voidD.46 *) + 64B])
and similarly
  # __for_begin_8 = PHI <(2), __for_begin_10(4)>
  if (__for_begin_8 == [(voidD.46 *) + 64B])
for the variable we've kept as variable, not alias, or something is upset by
the const variable without initializer.

The alias has been created and its DECL_INITIAL cleared by sem_variable::merge,
2265  DECL_INITIAL (alias->decl) = NULL;
2266  ((symtab_node *)alias)->call_for_symbol_and_aliases
(clear_decl_rtl,
2267   NULL, true);
2268  alias->need_bounds_init = false;
2269  alias->remove_all_references ();
2270  if (TREE_ADDRESSABLE (alias->decl))
2271original->call_for_symbol_and_aliases (set_addressable, NULL,
true);
2272
2273  varpool_node::create_alias (alias_var->decl, decl);
2274  alias->resolve_alias (original);
2275
2276  if (dump_file)
2277fprintf (dump_file, "Unified; Variable alias has been
created.\n");
Not really sure if it isn't an optimization problem if optimizers don't see the
initializer (if they are smart enough to look through the alias).

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #6 from Martin Liška  ---
(In reply to Jakub Jelinek from comment #3)
> Seems the inliner immediately undoes what ICF did and both get inlined into
> main as well.

It's not undoing the decision because the symbol (Bar) is global. So it both
inlines into main and the wrapper is preserved. But it's not triggering the
issue.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #5 from Martin Liška  ---
Created attachment 43546
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=43546=edit
Problematic CCP2 dump file

Jakub do you understand why is the folding happens? I'm not skilled in CCP.

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

Martin Liška  changed:

   What|Removed |Added

   Assignee|unassigned at gcc dot gnu.org  |marxin at gcc dot 
gnu.org

--- Comment #4 from Martin Liška  ---
Yes, it's cpp2 that removes the check, IPA inlining is not needed:

$ cat ~/Programming/testcases/pr84658.cpp
#include 

const int kTestCasesFoo[] = { 0, 1, 2, 3, 4, 5, 8, 15, 16, 17, 512, 1020, 1021,
1022, 1023, 1024 };
const int kTestCasesBar[] = { 0, 1, 2, 3, 4, 5, 8, 15, 16, 17, 512, 1020, 1021,
1022, 1023, 1024 };

void Foo() {
for (int count : kTestCasesFoo) {
printf("foo: %d\n", count);
}
}

void Bar() {
for (int count : kTestCasesBar) {
printf("bar: %d\n", count);
}
}

int main() {
Foo();
Bar();
}

$ ./xg++ -B. ~/Programming/testcases/pr84658.cpp -O2 -fno-ipa-icf-functions
-fmerge-all-constants -c -fno-inline

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

--- Comment #3 from Jakub Jelinek  ---
Seems the inliner immediately undoes what ICF did and both get inlined into
main as well.
The aD.2373 array becomes an alias of aD.2363.
And the real bug is introduced in ccp2:
Folding predicate __for_begin_5 == [(void *) + 64B] to 0
on previously:
  intD.9 bD.2396;
  const intD.9 * __for_beginD.2397;
  static const intD.9 aD.2363[16] = {0, 1, 2, 3, 4, 5, 8, 15, 16, 17, 512,
1020, 1021, 1022, 1023, 1024};
  intD.9 bD.2394;
  const intD.9 * __for_beginD.2395;
  static const intD.9 aD.2373[16];

   [local count: 63136019]:
  printf ("foo ");

   [local count: 1073741820]:
  # __for_begin_8 = PHI <(2), __for_begin_10(4)>
  if (__for_begin_8 == [(voidD.46 *) + 64B])
goto ; [5.88%]
  else
goto ; [94.12%]

   [local count: 1010605800]:
  b_9 = *__for_begin_8;
  printf ("%d, ", b_9);
  __for_begin_10 = __for_begin_8 + 4;
  goto ; [100.00%]

   [local count: 63136019]:
  printf ("end\nbar ");

   [local count: 1073741825]:
  # __for_begin_5 = PHI <(5), __for_begin_7(7)>
  if (__for_begin_5 == [(voidD.46 *) + 64B])
goto ; [5.88%]
  else
goto ; [94.12%]

   [local count: 1010605805]:
  b_6 = *__for_begin_5;
  printf ("%d, ", b_6);
  __for_begin_7 = __for_begin_5 + 4;
  goto ; [100.00%]

   [local count: 63136019]:
  __builtin_puts (&"end"[0]);

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

Jakub Jelinek  changed:

   What|Removed |Added

 CC||jakub at gcc dot gnu.org

--- Comment #2 from Jakub Jelinek  ---
Adjusted testcase for the testsuite:
// PR ipa/84658
// { dg-do run }
// { dg-options "-O3 -fmerge-all-constants" }
// { dg-output "foo 0, 1, 2, 3, 4, 5, 8, 15, 16, 17, 512, 1020, 1021, 1022,
1023, 1024, end.*" }
// { dg-output "bar 0, 1, 2, 3, 4, 5, 8, 15, 16, 17, 512, 1020, 1021, 1022,
1023, 1024, end" }

extern "C" int printf (const char *, ...);

void
foo ()
{
  const int a[] = { 0, 1, 2, 3, 4, 5, 8, 15, 16, 17, 512, 1020, 1021, 1022,
1023, 1024 };
  for (int b : a)
printf ("%d, ", b);
}

void
bar ()
{
  const int a[] = { 0, 1, 2, 3, 4, 5, 8, 15, 16, 17, 512, 1020, 1021, 1022,
1023, 1024 };
  for (int b : a)
printf ("%d, ", b);
}

int
main ()
{
  printf ("foo ");
  foo ();
  printf ("end\nbar ");
  bar ();
  printf ("end\n");
}

[Bug ipa/84658] [7/8 Regression] -O3 -fmerge-all-constants causes incorrect for-each loop generation.

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=84658

Richard Biener  changed:

   What|Removed |Added

   Priority|P3  |P2
 Status|UNCONFIRMED |NEW
  Known to work||6.4.1
   Keywords||wrong-code
   Last reconfirmed||2018-03-02
  Component|c++ |ipa
 CC||marxin at gcc dot gnu.org
 Ever confirmed|0   |1
Summary|-O3 -fmerge-all-constants   |[7/8 Regression] -O3
   |causes incorrect for-each   |-fmerge-all-constants
   |loop generation.|causes incorrect for-each
   ||loop generation.
   Target Milestone|--- |7.4

--- Comment #1 from Richard Biener  ---
Confirmed.  IPA ICF breaks this somehow, already visible with -O2 -fipa-icf.

Martin?