[Bug middle-end/95622] New: force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta-2.c

[Bug middle-end/95622] New: force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta-2.c

[burnus at gcc dot gnu.org]

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

            Bug ID: 95622
           Summary: force_output flag on a variable prevents optimization
                    / regresses
                    c-c++-common/goacc/kernels-alias-ipa-pta-2.c
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Keywords: missed-optimization, openacc, openmp
          Severity: normal
          Priority: P3
         Component: middle-end
          Assignee: unassigned at gcc dot gnu.org
          Reporter: burnus at gcc dot gnu.org
                CC: jakub at gcc dot gnu.org
  Target Milestone: ---

The following only shows up as FAIL in the testsuite if  ENABLE_OFFLOAD  is
true, even though it is only a compile-time test.   [Hence, it can make sense
to not only "make check-target-libgomp" with an offloading compiler but also
gcc/.]

Somewhat similar to PR 68773.

The testcase: c-c++-common/goacc/kernels-alias-ipa-pta-2.c
…
  {
    a[0] = 0;
    b[0] = 1;
    c[0] = a[0];
  }
…

Without ENABLE_OFFLOAD or before
r11-1075-g1c0fdaf79e3618fd7512608a2e5c62b6b306e9e8 (for PR94848 + PR95551):

  MEM[(unsigned int *)_5] = 0;  // a[0] = 0
  MEM[(unsigned int *)_4] = 1;  // b[0] = 1
  MEM[(unsigned int *)_3] = 0;  // c[0] = a[0]

With r11-1075 + offloading compiler, force_output is on offloading variables
(to ensure they are not optimized away), the last line becomes

  _8 = MEM[(unsigned int *)_5];  // _8 = a[0]
  MEM[(unsigned int *)_3] = _8;  // c[0] = _8

Expected: force_output does not affect the optimization. (Especially as "a"
cannot be optimized away as it is also used for "copyout".)

[A very well optimizing compiler could see that nothing uses a/b/c after the
target section and could replace the whole function body by "{ }"…]

[Bug middle-end/95622] force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta-2.c

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Dunno, perhaps there is a better way than force_output, how exactly we
represent
void *refs[] __attribute__((used)) = { &var1, &var2, &var3 };
where we say the array can't be optimized away and thus the referenced vars
can't be optimized away?  Does that also prevent such an optimization?
For these vars, all we want to ensure is that the vars aren't optimized away,
but don't need to derive from that that some other code could be accessing
those vars indirectly.
Or, shall we support putting NULL (or some other magic value) into the tables
on the host or offload side if those vars have been optimized away completely
on the host (or offload) sides?  At least when not doing the not yet supported
offloading from offload device back to host, perhaps a variable optimized away
completely on the host will mean we can't every lookup such variable's address
and so nothing needs to be recorded in the splay trees.  But, if something is
completely unused on the offload side but used on the host side, one can still
try to e.g. map those variables and should get the address of the device var
even if it is otherwise unused.

[Bug middle-end/95622] [11 Regression] force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta{-2,-4,}.c

[tschwinge at gcc dot gnu.org]

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

Thomas Schwinge <tschwinge at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|force_output flag on a      |[11 Regression]
                   |variable prevents           |force_output flag on a
                   |optimization  / regresses   |variable prevents
                   |c-c++-common/goacc/kernels- |optimization  / regresses
                   |alias-ipa-pta-2.c           |c-c++-common/goacc/kernels-
                   |                            |alias-ipa-pta{-2,-4,}.c
                 CC|                            |tschwinge at gcc dot gnu.org
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2020-06-12

--- Comment #2 from Thomas Schwinge <tschwinge at gcc dot gnu.org> ---
(In reply to Tobias Burnus from comment #0)
> The following

Specifically:

    PASS: c-c++-common/goacc/kernels-alias-ipa-pta-2.c (test for excess errors)
    [-PASS:-]{+FAIL:+} c-c++-common/goacc/kernels-alias-ipa-pta-2.c
scan-tree-dump-times optimized "(?n)= 0;$" 2
    PASS: c-c++-common/goacc/kernels-alias-ipa-pta-2.c scan-tree-dump-times
optimized "(?n)= 1;$" 1
    PASS: c-c++-common/goacc/kernels-alias-ipa-pta-2.c scan-tree-dump-times
optimized "(?n)= \\*a" 0

    PASS: c-c++-common/goacc/kernels-alias-ipa-pta-4.c (test for excess errors)
    [-PASS:-]{+FAIL:+} c-c++-common/goacc/kernels-alias-ipa-pta-4.c
scan-tree-dump-times optimized "(?n)= 0;$" 2
    PASS: c-c++-common/goacc/kernels-alias-ipa-pta-4.c scan-tree-dump-times
optimized "(?n)= 1;$" 1
    [-PASS:-]{+FAIL:+} c-c++-common/goacc/kernels-alias-ipa-pta-4.c
scan-tree-dump-times optimized "(?n)= \\*_[0-9];$" 0

    PASS: c-c++-common/goacc/kernels-alias-ipa-pta.c (test for excess errors)
    [-PASS:-]{+FAIL:+} c-c++-common/goacc/kernels-alias-ipa-pta.c
scan-tree-dump-times optimized "(?n)= 0;$" 2
    PASS: c-c++-common/goacc/kernels-alias-ipa-pta.c scan-tree-dump-times
optimized "(?n)= 1;$" 1
    PASS: c-c++-common/goacc/kernels-alias-ipa-pta.c scan-tree-dump-times
optimized "(?n)= \\*_[0-9]\\[0\\];$" 0

Same for C++.

Thus: "[11 Regression] force_output flag on a variable prevents optimization  /
regresses c-c++-common/goacc/kernels-alias-ipa-pta{-2,-4,}.c".

We'll need to analyze if we need to restore that functionality now, or if it's
OK to lose that optimization.

> only shows up as FAIL in the testsuite if  ENABLE_OFFLOAD  is
> true, even though it is only a compile-time test.   [Hence, it can make
> sense to not only "make check-target-libgomp" with an offloading compiler
> but also gcc/.]

Somewhat unfortunate, but yes.

The test cases need to be XFAILed conditionally "if  ENABLE_OFFLOAD  is true".

> Somewhat similar to PR 68773.
> 
> The testcase: c-c++-common/goacc/kernels-alias-ipa-pta-2.c
> …
>   {
>     a[0] = 0;
>     b[0] = 1;
>     c[0] = a[0];
>   }
> …
> 
> Without ENABLE_OFFLOAD or before
> r11-1075-g1c0fdaf79e3618fd7512608a2e5c62b6b306e9e8 (for PR94848 + PR95551):
> 
>   MEM[(unsigned int *)_5] = 0;  // a[0] = 0
>   MEM[(unsigned int *)_4] = 1;  // b[0] = 1
>   MEM[(unsigned int *)_3] = 0;  // c[0] = a[0]
> 
> With r11-1075 + offloading compiler, force_output is on offloading variables
> (to ensure they are not optimized away), the last line becomes
> 
>   _8 = MEM[(unsigned int *)_5];  // _8 = a[0]
>   MEM[(unsigned int *)_3] = _8;  // c[0] = _8

(... as is visible from the 'kernels-alias-ipa-pta-2.c.104t.fre3' dump
onwards.)

> Expected: force_output does not affect the optimization. (Especially as "a"
> cannot be optimized away as it is also used for "copyout".)
> 
> [A very well optimizing compiler could see that nothing uses a/b/c after the
> target section and could replace the whole function body by "{ }"…]

That's PR90591 etc.?

[Bug middle-end/95622] [11 Regression] force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta{-2,-4,}.c

[burnus at gcc dot gnu.org]

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

--- Comment #3 from Tobias Burnus <burnus at gcc dot gnu.org> ---
I think there are three related issues here:

(a) force_output = 1 prevents at least one optimization

(b) If not using force_output = 1, we need to find another way to
tell the compiler that the variable is actually needed – even if
unused. (cf. comment 1)

(c) There are missed optimizations related to offloading – both
before streaming-out the offload table and afterward.

 * * *

(See also PR 92029 for another test case fixed – but I think we cannot learn
much from that one as there the optimization happens before the stream out.)

(In reply to Thomas Schwinge from comment #2)
> We'll need to analyze if we need to restore that functionality now, or if
> it's OK to lose that optimization.

Fixing "now" is only possible by reverting the patch. But given that the patch
fixes a wrong code generation (link errors or ICE) and comes after the first
optimizations have been done (before LTO streamout), I would not undo it. –
Hence: not fixing now but the more optimizations the better :-)

[In particular, my impression is that artificial, read-only static variables do
not need to be linked between host and device as one cannot really take their
address and they cannot be updated or changed.]

[Bug middle-end/95622] [11 Regression] force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta{-2,-4,}.c

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tobias Burnus <burnus@gcc.gnu.org>:

https://gcc.gnu.org/g:e70fbee1a3dd9fdeb661366dbf3096d76c4e2d6e

commit r11-1384-ge70fbee1a3dd9fdeb661366dbf3096d76c4e2d6e
Author: Tobias Burnus <tobias@codesourcery.com>
Date:   Tue Jun 16 15:14:13 2020 +0200

    testsuite: Add offloading_enabled check and use it for xfail (PR95622)

    gcc/testsuite/ChangeLog:

            PR middle-end/95622
            * lib/target-supports.exp
(check_effective_target_offloading_enabled):
            New.
            * c-c++-common/goacc/kernels-alias-ipa-pta-2.c: Use it for xfail.
            * c-c++-common/goacc/kernels-alias-ipa-pta-4.c: Likewise.
            * c-c++-common/goacc/kernels-alias-ipa-pta.c: Likewise.

    Reviewed-by: Thomas Schwinge <thomas@codesourcery.com>

[Bug middle-end/95622] [11 Regression] force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta{-2,-4,}.c

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Tobias Burnus
<burnus@gcc.gnu.org>:

https://gcc.gnu.org/g:7d249d84e4c1be7c6f857c3a9d7791623a0e9d00

commit r10-8309-g7d249d84e4c1be7c6f857c3a9d7791623a0e9d00
Author: Tobias Burnus <tobias@codesourcery.com>
Date:   Tue Jun 16 15:14:13 2020 +0200

    testsuite: Add offloading_enabled check and use it for xfail (PR95622)

    gcc/testsuite/ChangeLog:

            PR middle-end/95622
            * lib/target-supports.exp
(check_effective_target_offloading_enabled):
            New.
            * c-c++-common/goacc/kernels-alias-ipa-pta-2.c: Use it for xfail.
            * c-c++-common/goacc/kernels-alias-ipa-pta-4.c: Likewise.
            * c-c++-common/goacc/kernels-alias-ipa-pta.c: Likewise.

    Reviewed-by: Thomas Schwinge <thomas@codesourcery.com>
    (cherry picked from commit e70fbee1a3dd9fdeb661366dbf3096d76c4e2d6e)

[Bug middle-end/95622] [11 Regression] force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta{-2,-4,}.c

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
            Version|10.0                        |11.0
   Target Milestone|---                         |11.0

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
not sure if fixed?

[Bug middle-end/95622] [11 Regression] force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta{-2,-4,}.c

[burnus at gcc dot gnu.org]

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

--- Comment #7 from Tobias Burnus <burnus at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #6)
> not sure if fixed?

Not fixed – only XFAILed.

The issue is that optimizations are not done with "node->force_output". As in
the example in comment 0:
  "a = 0; c = a"  will become
    "c[0] = 0"    without force_output
but "c[0] = a[0]"    with force_output set.

As the variable has been already used in the offload-variable table, it cannot
be removed for good – but not using it is fine.

[Bug middle-end/95622] [11 Regression] force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta{-2,-4,}.c

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
           Keywords|                            |deferred

[Bug middle-end/95622] [11 Regression] force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta{-2,-4,}.c

[burnus at gcc dot gnu.org]

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

--- Comment #8 from Tobias Burnus <burnus at gcc dot gnu.org> ---
I am not sure whether this is a sensible solution, but it fixes
the issue for c-c++-common/goacc/kernels-alias-ipa-pta-2.c ...


diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
index 529ec3a5b80..c93e9b46d8d 100644
--- a/gcc/tree-ssa-structalias.c
+++ b/gcc/tree-ssa-structalias.c
@@ -8132,7 +8132,7 @@ refered_from_nonlocal_fn (struct cgraph_node *node, void
*data)
   *nonlocal_p |= (node->used_from_other_partition
                  || DECL_EXTERNAL (node->decl)
                  || TREE_PUBLIC (node->decl)
-                 || node->force_output
+                 || (node->force_output && !node->offloadable)
                  || lookup_attribute ("noipa", DECL_ATTRIBUTES (node->decl)));
   return false;
 }
@@ -8195,7 +8195,7 @@ ipa_pta_execute (void)
       bool nonlocal_p = (node->used_from_other_partition
                         || DECL_EXTERNAL (node->decl)
                         || TREE_PUBLIC (node->decl)
-                        || node->force_output
+                        || (node->force_output && !node->offloadable)
                         || lookup_attribute ("noipa",
                                              DECL_ATTRIBUTES (node->decl)));
       node->call_for_symbol_thunks_and_aliases (refered_from_nonlocal_fn,

[Bug middle-end/95622] [11 Regression] force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta{-2,-4,}.c

[rguenther at suse dot de]

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

--- Comment #9 from rguenther at suse dot de <rguenther at suse dot de> ---
On Thu, 25 Mar 2021, burnus at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95622
> 
> --- Comment #8 from Tobias Burnus <burnus at gcc dot gnu.org> ---
> I am not sure whether this is a sensible solution, but it fixes
> the issue for c-c++-common/goacc/kernels-alias-ipa-pta-2.c ...
> 
> 
> diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
> index 529ec3a5b80..c93e9b46d8d 100644
> --- a/gcc/tree-ssa-structalias.c
> +++ b/gcc/tree-ssa-structalias.c
> @@ -8132,7 +8132,7 @@ refered_from_nonlocal_fn (struct cgraph_node *node, void
> *data)
>    *nonlocal_p |= (node->used_from_other_partition
>                   || DECL_EXTERNAL (node->decl)
>                   || TREE_PUBLIC (node->decl)
> -                 || node->force_output
> +                 || (node->force_output && !node->offloadable)
>                   || lookup_attribute ("noipa", DECL_ATTRIBUTES (node->decl)));
>    return false;
>  }
> @@ -8195,7 +8195,7 @@ ipa_pta_execute (void)
>        bool nonlocal_p = (node->used_from_other_partition
>                          || DECL_EXTERNAL (node->decl)
>                          || TREE_PUBLIC (node->decl)
> -                        || node->force_output
> +                        || (node->force_output && !node->offloadable)
>                          || lookup_attribute ("noipa",
>                                               DECL_ATTRIBUTES (node->decl)));
>        node->call_for_symbol_thunks_and_aliases (refered_from_nonlocal_fn,

It's for sure incorrect since node->force_output could be set for other
reasons.  But of course the reason for the node->force_output check
is that it is set by __attribute__((used)) which means a function/variable
can be invoked/accessed by means not visible to the compiler.

As said elsewhere the "fix" is to make the offload reference visible
to IPA early enough.

[Bug middle-end/95622] [11/12 Regression] force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta{-2,-4,}.c

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|11.0                        |11.2

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 11.1 has been released, retargeting bugs to GCC 11.2.

[Bug middle-end/95622] [11/12 Regression] force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta{-2,-4,}.c

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|11.2                        |11.3

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 11.2 is being released, retargeting bugs to GCC 11.3

[Bug middle-end/95622] [11/12 Regression] force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta{-2,-4,}.c

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|11.3                        |11.4

--- Comment #12 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 11.3 is being released, retargeting bugs to GCC 11.4.

[Bug middle-end/95622] [11/12/13/14 Regression] force_output flag on a variable prevents optimization / regresses c-c++-common/goacc/kernels-alias-ipa-pta{-2,-4,}.c

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|11.4                        |11.5

--- Comment #13 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 11.4 is being released, retargeting bugs to GCC 11.5.