[Bug c/59193] Unused postfix operator temporaries

[Bug c/59193] Unused postfix operator temporaries

[joseph at codesourcery dot com]

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

--- Comment #1 from joseph at codesourcery dot com <joseph at codesourcery dot com> ---
I see nothing at http://gcc.gnu.org/codingconventions.html that would 
apply such a C++ coding practice to the mostly C-like code in GCC.  
Changes to coding conventions should be proposed on the gcc@gcc.gnu.org 
mailing list, not in Bugzilla; the mailing list is a better place to get 
consensus on such things and Bugzilla is better for things that are 
unambiguously bugs rather than stylistic or possibly controversial.

Personally I think such changes are pointless when ordinary C integer / 
pointer types are in use.

[Bug c/59193] Unused postfix operator temporaries

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
   Last reconfirmed|                            |2013-11-20
     Ever confirmed|0                           |1

[Bug c/59193] Unused postfix operator temporaries

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |RESOLVED
                 CC|                            |mpolacek at gcc dot gnu.org
         Resolution|---                         |INVALID

[Bug c/59193] Unused postfix operator temporaries

[mtewoodbury at gmail dot com]

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

Max TenEyck Woodbury <mtewoodbury at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |---

--- Comment #2 from Max TenEyck Woodbury <mtewoodbury at gmail dot com> ---
The practice is very common in C (and the GCC code) and is NOT peculiar to C++.

The creation of temporary values that are never used is a waste of resources
and, even when removed by the optimizer, represent an, admittedly minor,
defect.
This may be a minor point but it is NOT controversial.  Also, it is not really
a
matter of style.  Your lack of insight on this is somewhat disturbing.  Marking
the argument as INVALID is just plain wrong.  It should be left open to provide
a reference for patches that address this problem.

[Bug c/59193] Unused postfix operator temporaries

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |INVALID

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
a++ and ++a should be treated as similar and don't change the semantics of the
loading from the variable or increase the number of loads if never used for
scalar types.  Now in C++, they are different when you overload them for
classes but we don't use that feature yet.

[Bug c/59193] Unused postfix operator temporaries

[mtewoodbury at gmail dot com]

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

Max TenEyck Woodbury <mtewoodbury at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |---

--- Comment #4 from Max TenEyck Woodbury <mtewoodbury at gmail dot com> ---
There is another semantic element to the a++ operator; the temporary where the
original value of the variable is stored until used.  You are either unaware of
this aspect of the language or are deliberately ignoring it.

This has NOTHING to do with overloading the postfix operators.  This is part of 
the basic definition of the C language.  In fact, C does NOT have operator
overloading and the majority of GCC is written in C, not C++.

Without optimization, this temporary store is allocated and set even if the
value is never used.  The fact that to value is not used and need never be set
is detected during optimization.  The fact that the temporary store is then
never
used and can be eliminated requires further optimization.  All this extra work
is unnecessary in well written code.  That makes the use of a++ where ++a will
do a minor coding defect.  This bug report provides a referent for changes that
improve that kind of defective code.

Since there are hundreds, if not thousands of instances of this defect in the
GCC code and there is no urgency in correcting these defects, this bug will
only
get resolved slowly.  Closing it for invalid reasons does the community a
disservice.

[Bug c/59193] Unused postfix operator temporaries

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |INVALID

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Take:
int f(int a)
{
  a++;
  return a;
}

int g(int a)
{
  ++a;
  return a;
}
------------------- CUT ------------------- 

The gimplifier produces the exact same IR for both cases:
f (int a)
{
  int D.1790;

  a = a + 1;
  D.1790 = a;
  return D.1790;
}


g (int a)
{
  int D.1792;

  a = a + 1;
  D.1792 = a;
  return D.1792;
}
------------------- CUT ------------------- 

So the compiler is already smart enough to remove the "temporary storage" even
at -O0.

With:
int f(int a)
{
  int b = a++;
  return a;
}
It does not remove it but that is because the result of a++ is not unused.

So it is the gimplifier knows if the result is unused and will not use them
otherwise.  This is the same issue as memcpy and its return value.

[Bug c/59193] Unused postfix operator temporaries

[jakub at gcc dot gnu.org]

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

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

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Also note that even at -O0, at no point during compilation with GCC/G++ if the
value of ++a or a++ isn't used and a has integral/pointer type one form is more
efficient than the other.  It is just a different tree code
({PRE,POST}{IN,DE}CREMENT_EXPR), but with the same operand, type etc.
So, there is no waste of any resources, it is not a defect to use either style,
it is purely coding convention matter.

[Bug c/59193] Unused postfix operator temporaries

[mtewoodbury at gmail dot com]

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

Max TenEyck Woodbury <mtewoodbury at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |---
           Severity|minor                       |trivial

--- Comment #8 from Max TenEyck Woodbury <mtewoodbury at gmail dot com> ---
Re: Manuel López-Ibáñez

Yes, as soon as I feel that my efforts will not be ignored, as they have been
so far...

Re:  Jakub Jelinek

You admit that different 'tree codes' are generated, so there is a difference
between the two sequences.  You further admit that -O0 does do some
optimization, otherwise the machine code emitted would store and discard
the temporary value because that is what the language standard says should
happen.  While this is quite trivial in practice, it shows that you place
your opinion over the exact semantics called for by the standard and lack
the ability to see that.  That is distinctly disturbing.
>From gcc-bugs-return-444286-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org Thu Feb 20 02:52:34 2014
Return-Path: <gcc-bugs-return-444286-listarch-gcc-bugs=gcc.gnu.org@gcc.gnu.org>
Delivered-To: listarch-gcc-bugs@gcc.gnu.org
Received: (qmail 3612 invoked by alias); 20 Feb 2014 02:52:34 -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 3580 invoked by uid 55); 20 Feb 2014 02:52:31 -0000
From: "hubicka at ucw dot cz" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug middle-end/58555] [4.9 Regression] Floating point exception in want_inline_self_recursive_call_p
Date: Thu, 20 Feb 2014 02:52:00 -0000
X-Bugzilla-Reason: CC
X-Bugzilla-Type: changed
X-Bugzilla-Watch-Reason: None
X-Bugzilla-Product: gcc
X-Bugzilla-Component: middle-end
X-Bugzilla-Version: 4.9.0
X-Bugzilla-Keywords:
X-Bugzilla-Severity: normal
X-Bugzilla-Who: hubicka at ucw dot cz
X-Bugzilla-Status: ASSIGNED
X-Bugzilla-Priority: P1
X-Bugzilla-Assigned-To: hubicka at gcc dot gnu.org
X-Bugzilla-Target-Milestone: 4.9.0
X-Bugzilla-Flags:
X-Bugzilla-Changed-Fields:
Message-ID: <bug-58555-4-D29bZxCpnv@http.gcc.gnu.org/bugzilla/>
In-Reply-To: <bug-58555-4@http.gcc.gnu.org/bugzilla/>
References: <bug-58555-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-02/txt/msg02043.txt.bz2
Content-length: 4421

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

--- Comment #25 from Jan Hubicka <hubicka at ucw dot cz> ---
> It is easier to just return at beggining instead of duplicating the check.
> Have patch for it, just for some reason
> I wanted to look deper into why we inline here.  I forgot the reason, but will
> work it out again today.
OK, remember the reason, the inlining decisions did not make sense :)

It turned out to be very basic and apparently ages old profile updating but in
clone_inlined_nodes. This function is used when one inlines call a->b
and it produces clone of b to be inlined into a.  It updates frequencies in
the clone, so if the edge is, say, executing twice per invocation of a,
the frequencies in b are doubled.

Now it may happen that b has inlined functions to it. In that case the function
recurses and inlines further. It stil updates frequencies on the same basis:
i.e. it takes average number of executions of edge per invocation and multiply.
This is completely wrong.  If there is chain b->c->d->e all inlined and c is
executed at average 10 times per execution of b and c->d, the the frequenceis
of edge c->d->e are already scaled up 10fold.
The current logic sees edge c->d already multiplied and scale again causing
quite an explosion in frequencies.

Bootstrapping/regtesting x86_64-linux.

Index: ipa-inline.c
==================================================================--- ipa-inline.c    (revision 207870)
+++ ipa-inline.c    (working copy)
@@ -708,6 +684,12 @@
   if (outer_node->global.inlined_to)
     caller_freq = outer_node->callers->frequency;

+  if (!caller_freq)
+    {
+      reason = "function is inlined and ulikely";
+      want_inline = false;
+    }
+
   if (!want_inline)
     ;
   /* Inlining of self recursive function into copy of itself within other
function
Index: ipa-inline.h
==================================================================--- ipa-inline.h    (revision 207870)
+++ ipa-inline.h    (working copy)
@@ -233,7 +234,8 @@
 /* In ipa-inline-transform.c  */
 bool inline_call (struct cgraph_edge *, bool, vec<cgraph_edge_p> *, int *,
bool);
 unsigned int inline_transform (struct cgraph_node *);
-void clone_inlined_nodes (struct cgraph_edge *e, bool, bool, int *);
+void clone_inlined_nodes (struct cgraph_edge *e, bool, bool, int *,
+              int freq_scale = -1);

 extern int ncalls_inlined;
 extern int nfunctions_inlined;
Index: ipa-inline-transform.c
==================================================================--- ipa-inline-transform.c    (revision 207870)
+++ ipa-inline-transform.c    (working copy)
@@ -127,11 +127,16 @@
    the edge and redirect it to the new clone.
    DUPLICATE is used for bookkeeping on whether we are actually creating new
    clones or re-using node originally representing out-of-line function call.
-   */
+   By default the offline copy is removed, when it appers dead after inlining.
+   UPDATE_ORIGINAL prevents this transformation.
+   If OVERALL_SIZE is non-NULL, the size is updated to reflect the
+   transformation.
+   FREQ_SCALE is implicit parameter used for internal bookeeping when
+   recursively copying functions inlined into the clone.  */

 void
 clone_inlined_nodes (struct cgraph_edge *e, bool duplicate,
-             bool update_original, int *overall_size)
+             bool update_original, int *overall_size, int freq_scale)
 {
   struct cgraph_node *inlining_into;
   struct cgraph_edge *next;
@@ -171,12 +176,16 @@
       duplicate = false;
       e->callee->externally_visible = false;
           update_noncloned_frequencies (e->callee, e->frequency);
+      gcc_assert (freq_scale == -1);
     }
       else
     {
       struct cgraph_node *n;
+
+      if (freq_scale == -1)
+        freq_scale = e->frequency;
       n = cgraph_clone_node (e->callee, e->callee->decl,
-                 e->count, e->frequency, update_original,
+                 e->count, freq_scale, update_original,
                  vNULL, true, inlining_into);
       cgraph_redirect_edge_callee (e, n);
     }
@@ -191,7 +200,7 @@
     {
       next = e->next_callee;
       if (!e->inline_failed)
-        clone_inlined_nodes (e, duplicate, update_original, overall_size);
+        clone_inlined_nodes (e, duplicate, update_original, overall_size,
freq_scale);
       if (e->speculative && !speculation_useful_p (e, true))
     {
       cgraph_resolve_speculation (e, NULL);

[Bug c/59193] Unused postfix operator temporaries

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |INVALID

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Max TenEyck Woodbury from comment #8)
> 
> Re:  Jakub Jelinek
> 
> You admit that different 'tree codes' are generated, so there is a difference
> between the two sequences.  You further admit that -O0 does do some
> optimization, otherwise the machine code emitted would store and discard
> the temporary value because that is what the language standard says should
> happen.  While this is quite trivial in practice, it shows that you place
> your opinion over the exact semantics called for by the standard and lack
> the ability to see that.  That is distinctly disturbing.

The language semantics != middle-end semantics.  There is no temporary variable
if the value is unused and will never be as it is "expanded" to ignore it if
the resulting value is unused.  This is the middle-end semantics of these tree
codes.  If the front-end wants the temporary variable always then it needs to
do the assignment.  The language semantics does not require a temporary
variable to hold the value if the value is unused so the middle-end semantics
of these tree codes fit the bill.

[Bug c/59193] Unused postfix operator temporaries

[mtewoodbury at gmail dot com]

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

Max TenEyck Woodbury <mtewoodbury at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |---

--- Comment #10 from Max TenEyck Woodbury <mtewoodbury at gmail dot com> ---
There is no VARIABLE, just a TEMPORARY r-value like  all the others that hold
intermediate results.

Also, the LANGUAGE semantics has the operator produce a result, an r-value,
that
has to be represented in some manner, that is, it has a store of some kind.
The machine code generated without optimization is required to put that result
into the store before incrementing the specified l-value. (sub-clause 6.2.5.4)
Optimization is allowed to, but not required to, remove such operations as long
as the change produces no detectable change in the program's results.

Now, stop misrepresenting the standard.  It makes your other pronouncements
less credible.

To go over this again, if a piece of code specifies a postfix operation
conceptually, the original value is stored somewhere.  That stored value is
then
discarded.   Those steps are extraneous and the code would be conceptually
cleaner without them.  As such, their present is a defect, a trivial defect,
but
still a defect.  Using the prefix operator in its place improves to code, again
trivially, but it does improve it.  Such changes may want to cite something as
justification for the change.  This report is such a justification.  Until all
such defects have been removed, it should stay open.

[Bug c/59193] Unused postfix operator temporaries

[pinskia at gcc dot gnu.org]

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

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |INVALID

--- Comment #11 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Max TenEyck Woodbury from comment #10)
> There is no VARIABLE, just a TEMPORARY r-value like  all the others that hold
> intermediate results.

Yes and temporary rvalues can be removed if unused.

> 
> Also, the LANGUAGE semantics has the operator produce a result, an r-value,
> that
> has to be represented in some manner, that is, it has a store of some kind.
> The machine code generated without optimization is required to put that
> result
> into the store before incrementing the specified l-value. (sub-clause
> 6.2.5.4)
> Optimization is allowed to, but not required to, remove such operations as
> long
> as the change produces no detectable change in the program's results.

Yes but rvalues are can be removed as unused.

> 
> Now, stop misrepresenting the standard.  It makes your other pronouncements
> less credible.

I am not misrepresenting the standard here as there is a conversion between
lvalue and rvalue here.

> 
> To go over this again, if a piece of code specifies a postfix operation
> conceptually, the original value is stored somewhere.  That stored value is
> then
> discarded.   Those steps are extraneous and the code would be conceptually
> cleaner without them.  As such, their present is a defect, a trivial defect,
> but
> still a defect.  Using the prefix operator in its place improves to code,
> again
> trivially, but it does improve it.  Such changes may want to cite something
> as
> justification for the change.  This report is such a justification.  Until
> all
> such defects have been removed, it should stay open.

Again the rvalues can be removed as unused.  This is allowed in the standard
and so the semantics of the tree codes is that if the result is unused then
there is no rvalue that happens.

[Bug c/59193] Unused postfix operator temporaries

[mtewoodbury at gmail dot com]

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

Max TenEyck Woodbury <mtewoodbury at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |---

--- Comment #12 from Max TenEyck Woodbury <mtewoodbury at gmail dot com> ---
Just because the compiler is ALLOWED to discard the r-value as part of the
OPTIMIZATION process, does NOT mean that a program that calls for an EXTRANEOUS
temporary is without defect.  Such a program IS trivially defective and
removing that kind of defect should be allowed without comment.  Such changes
may want to cite an open bug report as their justification.

While you are arguing about what the compiler does, the issue here is about
what the source code calls for.  The compiler can and should clean up such
defects, but it should not be called upon to do so.  THAT i the issue and you
are NOT addressing it.

[Bug c/59193] Unused postfix operator temporaries

[mpolacek at gcc dot gnu.org]

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

Marek Polacek <mpolacek at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|---                         |INVALID

--- Comment #13 from Marek Polacek <mpolacek at gcc dot gnu.org> ---
Closing out now, BZ is not the right place to discuss coding conventions.