Bug 45307

Summary: Stores expanding to no RTL not removed by tree optimizers, Empty ctors/dtors not eliminated
Product: gcc Reporter: Jan Hubicka <hubicka>
Component: middle-endAssignee: Not yet assigned to anyone <unassigned>
Status: RESOLVED FIXED    
Severity: normal CC: gcc-bugs, jason, mark, meissner, mh+gcc, rguenth, tglek
Priority: P3    
Version: 4.6.0   
Target Milestone: ---   
Host: Target:
Build: Known to work:
Known to fail: Last reconfirmed: 2010-08-17 15:57:59
Attachments: testcase.

Description Jan Hubicka 2010-08-17 12:36:46 UTC
Compilling:
 ./g++ -O2 ~/mozalloc.ii  -S -m32
gives me:
        .type   _GLOBAL__I_moz_free, @function
_GLOBAL__I_moz_free:
.LFB58:
        .cfi_startproc
        rep
        ret
        .cfi_endproc
.LFE58:
        .size   _GLOBAL__I_moz_free, .-_GLOBAL__I_moz_free
        .section        .ctors,"aw",@progbits
        .align 4
        .long   _GLOBAL__I_moz_free
        .local  _ZN7mozillaL8fallibleE
        .comm   _ZN7mozillaL8fallibleE,1,1

I.e. an empty constructor function that can be eliminated and also 
local var   _ZN7mozillaL8fallibleE that is not used at all.
The reason is that constructor in .optimized stage looks like:
(static initializers for /mozilla-central/memory/mozalloc/mozalloc.cpp) ()
{
<bb 2>:
  fallible = {};
  return;

}

that later expands to nothing.  If we optimized this away during early opts I already have patch that makes it easy to eliminate all global ctors/dtors that are known to be pure.

Honza
Comment 1 Jan Hubicka 2010-08-17 12:38:06 UTC
Created attachment 21503 [details]
testcase.
Comment 2 Richard Biener 2010-08-17 12:49:22 UTC
It's the FEs job really.
Comment 3 Jan Hubicka 2010-08-17 13:34:34 UTC
Adding Mark and Jason to CC then, but Jakub seems right about -Wuninitialized warnings.
Comment 4 Mark Mitchell 2010-08-17 14:41:43 UTC
I have no object to the FE removing trivial code if it can do so -- but I also think that the middle-end should be able to deduce that a function is pure later in the process and eliminate it then.

I don't understand Richard's comment about -Wuninitialized; I don't see any comments from Jakub in this PR.
Comment 5 Richard Biener 2010-08-17 15:57:59 UTC
I also wonder why we can't remove such stores based on ipa-reference analysis.

Reduced testcase:

struct __attribute__ ((visibility ("default"))) fallible_t { };
const fallible_t fallible = fallible_t();
Comment 6 Jan Hubicka 2010-08-17 16:42:55 UTC
We are not removing write only vars because richi told me that there is no convenient way to tell aliasing that variable is write only.  It is easy to detect for me, but bit harder to get rid of them, since at IPA pass I can't modify body so I need to pass it down to local optimization and remove only then.
I guess I will add the feature (detecting them and removing the statements in transform pass). It just seemed not worth the hassle when I was looking into it originally.

Concerning Mark's question, we briefly discussed this on IRC

So we now have

1) C++ frontend probably should not produce static constructor for empty struct at all
2) We should be able to get rid of writes to empty structs in backend
3) Middle end should take away pure constructors
4) We should be able to optimize away write only variables

I will work on 3) and probably 4) later.

Mark, why C++ is not doing 1)? 
Comment 7 Jan Hubicka 2010-08-17 16:45:49 UTC
oops, I've left IRC sentence unfinished.  At IRC Richi told that FEs should not ever produce stores to empty structs at first place, so we don't need middle end logic of taking them away.  Jakub thought that those stores are left to keep -Wuninitialized safe.
Comment 8 Jakub Jelinek 2010-08-17 16:53:47 UTC
Perhaps related to PR43075, before that last commit there genericization was removing empty struct assignments.
Comment 9 Jason Merrill 2010-08-17 17:41:14 UTC
But that change was largely reversed by the fix for PR 43787.
Comment 10 Mark Mitchell 2010-08-17 17:50:58 UTC
The problem with -Wuninitialized might be the same problem I've been sermonizing about for years -- we're trying to issue sensible warnings from our optimizers, which means that as the optimizers are perturbed, the set of warnings will change in various ways.  But, there's no reason for -Wuninitialized to warn about a use of an empty structure, so I don't see why removing the stores is unreasonable.

I certainly think that the front-end could safely optimize away such stores, and that it would be reasonably easy to do so in the trivial cases.  (Obviously, the optimizers would be responsible for more complex cases where things are not perhaps immediately obvious to the front-end.)  I don't think there's any reason we don't do that in the front-end; it's just something we haven't done.
Comment 11 Jason Merrill 2010-08-17 18:09:09 UTC
There are two issues here:

1) expand_static_init decides whether a variable needs static initialization before gimplification, and
2) Richard's MEM_REF-related change to cp_gimplify_expr caused us to stop removing the copy in this example.

It sounds like if we fix #2 then the patch Honza mentions having should do the trick.  I'll do that.
Comment 12 Jason Merrill 2010-08-17 18:18:08 UTC
Actually, Richard's change didn't affect this; we were already missing it because of the complex interoperation of cp_gimplify_expr and gimplify_modify_expr_rhs.
Comment 13 Jason Merrill 2010-08-19 17:01:14 UTC
Subject: Bug 45307

Author: jason
Date: Thu Aug 19 17:00:51 2010
New Revision: 163380

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=163380
Log:
	PR c++/45307
	* gimplify.c (gimplify_init_constructor): Just return GS_UNHANDLED
	if ctor is empty.
	(gimplify_modify_expr_rhs): Adjust.

Added:
    trunk/gcc/testsuite/g++.dg/tree-ssa/empty-2.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/cp-gimplify.c
    trunk/gcc/gimplify.c
    trunk/gcc/testsuite/ChangeLog

Comment 14 Jan Hubicka 2010-08-20 10:23:15 UTC
http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01544.html
has patch for empty constructor removal in middle end.
Comment 15 Jan Hubicka 2010-08-21 09:46:34 UTC
Subject: Bug 45307

Author: hubicka
Date: Sat Aug 21 09:46:15 2010
New Revision: 163439

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=163439
Log:

	PR c++/45307
	PR c++/17736
	* cgraph.h (cgraph_only_called_directly_p,
	cgraph_can_remove_if_no_direct_calls_and_refs_p): Handle
	static cdtors.
	* cgraphunit.c (cgraph_decide_is_function_needed): Static cdtors
	are not needed.
	(cgraph_finalize_function): Static cdtors are reachable.
	(cgraph_mark_functions_to_output): Use cgraph_only_called_directly_p.

	* gcc.dg/ipa/ctor-empty-1.c: Add testcase.
	* g++.dg/tree-ssa/empty-2.C: Check that constructor got optimized out.


Added:
    trunk/gcc/testsuite/gcc.dg/ipa/ctor-empty-1.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cgraph.c
    trunk/gcc/cgraph.h
    trunk/gcc/cgraphunit.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/g++.dg/tree-ssa/empty-2.C

Comment 16 Jan Hubicka 2010-08-21 09:48:00 UTC
Empty constructors are now optimized away.  We still should get removal of write only global vars.
Comment 17 Jan Hubicka 2014-09-26 16:34:05 UTC
Fixed.