[Bug tree-optimization/60973] New: Invalid propagation of a tail call in copyrename2 pass

[Bug tree-optimization/60973] New: Invalid propagation of a tail call in copyrename2 pass

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60973

            Bug ID: 60973
           Summary: Invalid propagation of a tail call in copyrename2 pass
           Product: gcc
           Version: 4.10.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ubizjak at gmail dot com

This problem can be illustrated with a target, that doesn't define
TARGET_ASM_CAN_OUTPUT_MI_THUNK, such as h8300-elf.

Compiling g++.dg/ipa/imm-devirt-2.C using -O2 with a crosscompiler to
h8300-elf, we get in _.037t.inline_param2:

virtual int C::_ZThn10_N1C3fooEi(int) (struct C * const this, int i)
{
  int retval___0;

  <bb 2>:
  this_2 = this_1(D) + 65526;
  retval___0_5 = C::foo (this_2, i_4(D)); [tail call]
  return retval___0_5;

}

and

int main(int, char**) (int argc, char * * argv)
{
  struct C c;
  int _3;
  int _6;

  <bb 2>:
  C::C (&c);
  _3 = get_input ();
  _6 = C::_ZThn10_N1C3fooEi (&c.D.1912.D.1895, _3);
  if (_6 != 4)
    goto <bb 3>;
  else
    goto <bb 4>;

  <bb 3>:
  abort ();

  <bb 4>:
  c ={v} {CLOBBER};
  return 0;

}

However, copyrename2 pass propagates tail call annotation to the call site,
resulting in _.054t.copyrename2:

int main(int, char**) (int argc, char * * argv)
{
  int retval___0;
  int D.2161;
  struct C * const this;
  struct C c;
  int _3;
  int _6;

  <bb 2>:
  C::C (&c);
  _3 = get_input ();
  this_7 = &c.D.1912.D.1895 + 65526;
  retval___0_8 = C::foo (this_7, _3); [tail call]          <<< here!
  _9 = retval___0_8;
  _6 = _9;
  if (_6 != 4)
    goto <bb 3>;
  else
    goto <bb 4>;

  <bb 3>:
  abort ();

  <bb 4>:
  c ={v} {CLOBBER};
  return 0;

}

On a target that support sibcalls, RTL expansion ends after tail call, so
_.170r.expand ends with:

...

(call_insn/u/j 15 14 16 2 (set (reg:SI 3 r3)
        (call (mem:SI (symbol_ref:SI ("_ZN1C3fooEi") [flags 0x3] 
<function_decl 0x7fe46eeab200 foo>) [0 foo S4 A32])
            (const_int 0 [0]))) -1
     (expr_list:REG_EH_REGION (const_int 0 [0])
        (nil))
    (expr_list:SI (use (reg:SI 4 r4))
        (expr_list:SI (use (reg:SI 3 r3))
            (nil))))
;;  succ:       EXIT [100.0%]  (ABNORMAL,SIBCALL)

(barrier 16 15 0)


The tail call is valid only in _ZThn16_N1C3fooEi (a.k.a non-virtual thunk to
C::foo(int)) and should not be propagated as a tail call to its call site.

[Bug ipa/60973] Invalid propagation of a tail call in devirt pass

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60973

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |wrong-code
             Target|                            |h8300-elf
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |jamborm at gcc dot gnu.org
          Component|tree-optimization           |ipa
            Summary|Invalid propagation of a    |Invalid propagation of a
                   |tail call in copyrename2    |tail call in devirt pass
                   |pass                        |

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
It's not copyrename (that's just the first dump you see it) but inlining.
Inlining probably needs to clear [tailcall] from all inlined stmts unless
it wants to prove the tailcall is still possible.

Thus,

Index: gcc/tree-inline.c
===================================================================
--- gcc/tree-inline.c   (revision 209782)
+++ gcc/tree-inline.c   (working copy)
@@ -1485,6 +1489,11 @@ remap_gimple_stmt (gimple stmt, copy_bod
       /* Create a new deep copy of the statement.  */
       copy = gimple_copy (stmt);

+      /* Clear flags that need revisiting.  */
+      if (is_gimple_call (copy)
+         && gimple_call_tail_p (copy))
+       gimple_call_set_tail (copy, false);
+
       /* Remap the region numbers for __builtin_eh_{pointer,filter},
         RESX and EH_DISPATCH.  */
       if (id->eh_map)

not sure if GF_CALL_FROM_THUNK needs similar handling.

The bug is probably not h8300-elf specific (but usually tailcall expansion
fails as it re-checks the validity of the transform - and IIRC that is
required, so it may even be a h8300-elf backend bug).

[Bug ipa/60973] Invalid propagation of a tail call in devirt pass

[hubicka at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60973

--- Comment #2 from Jan Hubicka <hubicka at gcc dot gnu.org> ---
I wonder if the tailcall flag can't be reliably set by tree-tailcall, but i
suppose we want tailcall in thunks even at -O0.
The patch seems fine to me.
Note that this is not ipa-devirt issue, just semi-latent bug, so this probably
should be backported to all release branches.

The effect of CALL_FROM_THUNK is the following:
          /* If we're compiling a thunk, pass through invisible references
             instead of making a copy.  */
          if (call_from_thunk_p
              || (callee_copies
                  && !TREE_ADDRESSABLE (type)
                  && (base = get_base_address (args[i].tree_value))
                  && TREE_CODE (base) != SSA_NAME
                  && (!DECL_P (base) || MEM_P (DECL_RTL (base)))))
            {

I am not really confident about this code, but I would say that if we inlined
the thunk itself, we still should have the copy around - at least I do not see
any code in tree-inline that would copy propagate this through.
So probably we do not need to make the another copy?  This may be true in
general for all parameters passed by invisible references that was passed to
caller same way and caller does not modify them or use after the call.  This
seems just special case of this optimization.

[Bug ipa/60973] Invalid propagation of a tail call in devirt pass

[ubizjak at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60973

--- Comment #3 from Uroš Bizjak <ubizjak at gmail dot com> ---
(In reply to Richard Biener from comment #1)

> Index: gcc/tree-inline.c

I have checked this patch on my target, where it fixes the runtime problem. The
optimized tree dump results in:

int main(int, char**) (int argc, char * * argv)
{
  int retval.0;
  struct C c;
  int _3;

  <bb 2>:
  C::C (&c);
  _3 = get_input ();
  retval.0_8 = C::foo (&c, _3);
  if (retval.0_8 != 4)
    goto <bb 3>;
  else
    goto <bb 4>;

  <bb 3>:
  abort ();

  <bb 4>:
  c ={v} {CLOBBER};
  return 0;

}

which expands to the correct RTL sequence.
>From gcc-bugs-return-450882-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Wed May 07 18:13:24 2014
Return-Path: <gcc-bugs-return-450882-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 12762 invoked by alias); 7 May 2014 18:13:24 -0000
Mailing-List: contact gcc-bugs-help@gcc.gnu.org; run by ezmlm
Precedence: bulk
List-Id: <gcc-bugs.gcc.gnu.org>
List-Archive: <http://gcc.gnu.org/ml/gcc-bugs/>
List-Post: <mailto:gcc-bugs@gcc.gnu.org>
List-Help: <mailto:gcc-bugs-help@gcc.gnu.org>
Sender: gcc-bugs-owner@gcc.gnu.org
Delivered-To: mailing list gcc-bugs@gcc.gnu.org
Received: (qmail 12702 invoked by uid 48); 7 May 2014 18:13:20 -0000
From: "hjl.tools at gmail dot com" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug c++/61082] [x86-64 Itanium ABI] g++ uses wrong return location for class with head padding
Date: Wed, 07 May 2014 18:13:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: c++
X-Bugzilla-Version: 4.9.0
X-Bugzilla-Keywords: ABI, wrong-code
X-Bugzilla-Severity: major
X-Bugzilla-Who: hjl.tools at gmail dot com
X-Bugzilla-Status: UNCONFIRMED
X-Bugzilla-Priority: P3
X-Bugzilla-Assigned-To: unassigned at gcc dot gnu.org
X-Bugzilla-Target-Milestone: ---
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-61082-4-HWoFZqVxqP@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-61082-4@http.gcc.gnu.org/bugzilla/>
References: <bug-61082-4@http.gcc.gnu.org/bugzilla/>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
X-Bugzilla-URL: http://gcc.gnu.org/bugzilla/
Auto-Submitted: auto-generated
MIME-Version: 1.0
X-SW-Source: 2014-05/txt/msg00574.txt.bz2
Content-length: 896

http://gcc.gnu.org/bugzilla/show_bug.cgi?ida082

--- Comment #14 from H.J. Lu <hjl.tools at gmail dot com> ---
(In reply to David Greene from comment #13)
> I see that 3.2.3 4 (b) is talking about considering adjacent fields in an
> eightbyte.  Is the intent to classify each eightbyte in an aggregate and
> then consider each eightbyte separately for assigning argument and return
> registers?

That is correct.

> The post-merger cleanup described in 5 appears to handle passing in memory
> in that it considers all of the fields in the aggregate together.
>
> Given the above, if a field crosses an eightbyte either it is larger than an
> eightbyte either it is unaligned which forces the whole argument to memory
> or its eightbytes are classified separately in a recursive manner.
>
> Does this sound correct?  If so, I think gcc is correct in what it's doing.

I think GCC is correct.

[Bug ipa/60973] Invalid propagation of a tail call in devirt pass

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60973

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

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

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
Before tunks we never bothered to compute [tailcall] before inlining
completed, but now explicitely setting the flag for thunks (and not letting
it be computed - why wouldn't that work?) breaks this.

So not setting the flag explicitely in expand_thunk looks like a better fix
to me?

[Bug ipa/60973] Invalid propagation of a tail call in devirt pass

[hubicka at ucw dot cz]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60973

--- Comment #5 from Jan Hubicka <hubicka at ucw dot cz> ---
> Before tunks we never bothered to compute [tailcall] before inlining
> completed, but now explicitely setting the flag for thunks (and not letting
> it be computed - why wouldn't that work?) breaks this.
> 
> So not setting the flag explicitely in expand_thunk looks like a better fix
> to me?

We always had this explicit set of tailcall in thunk expansion code -
originally
in C++ frontend and at early LTO times I just literaly moved it to cgraphunit.
This patch http://gcc.gnu.org/ml/gcc-patches/2013-09/msg01035.html makes it
possible that tunks are inlined since we lower them to gimple bodies early
and it is why things breaks now as inliner does not expect it.

My initial reaction (written in previously comment) was also that tailcall
should
discover the flags themself and we could avoid setting them in the thunk
expansion.
Sadly I think it is not quite the case; tailcall is very conservative and I
believe
it will give up in cases where thunks are possible.  Also it is not run at -O0
and for thunks we want the tailcall to happen since it only improves debugging
exprience and saves codegen time...

So I would probably say we should fix that in tree-inline as your patch
propose.

[Bug ipa/60973] Invalid propagation of a tail call in devirt pass

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2014-05-13
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
     Ever confirmed|0                           |1

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
Ok.

[Bug ipa/60973] Invalid propagation of a tail call in devirt pass

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
Author: rguenth
Date: Tue May 13 11:04:44 2014
New Revision: 210364

URL: http://gcc.gnu.org/viewcvs?rev=210364&root=gcc&view=rev
Log:
2014-05-13  Richard Biener  <rguenther@suse.de>

    PR ipa/60973
    * tree-inline.c (remap_gimple_stmt): Clear tail call flag,
    it needs revisiting whether the call still may be tail-called.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-inline.c

[Bug ipa/60973] Invalid propagation of a tail call in devirt pass

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED
   Target Milestone|---                         |4.9.1

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed for 4.9.1.

[Bug ipa/60973] Invalid propagation of a tail call in devirt pass

[rguenth at gcc dot gnu.org]

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

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
Author: rguenth
Date: Tue May 13 11:06:00 2014
New Revision: 210365

URL: http://gcc.gnu.org/viewcvs?rev=210365&root=gcc&view=rev
Log:
2014-05-13  Richard Biener  <rguenther@suse.de>

    PR ipa/60973
    * tree-inline.c (remap_gimple_stmt): Clear tail call flag,
    it needs revisiting whether the call still may be tail-called.

Modified:
    branches/gcc-4_9-branch/gcc/ChangeLog
    branches/gcc-4_9-branch/gcc/tree-inline.c