This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PR debug/47106] account used vars only once
On Mon, Jan 31, 2011 at 11:58 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Jan 21, 2011, Alexandre Oliva <aoliva@redhat.com> wrote:
>
>> we seem to be looking at default-initialized (not recomputed)
>> information, a possibility that IIUC you and honza were concered about
>> when reviewing the original patch, but that went way over the top of
>> my head :-(
>
> I ended up introducing some infrastructure to check that we don't use
> uninitialized “used” in var_ann, and that helped me catch a few
> problems.
>
> First, it reported that var decls created corresponding to result decls
> of inlined functions were added twice to the LOCAL_DECLs list. ?The
> first patch below fixes that.
>
> Second, it showed that we completely failed to compute used flags after
> inlining. ?At first, I split the computation of used flags out of
> remove_unused_locals() and refrain from actually removing variables, but
> then I decided calling removed_unused_locals() after inlining might get
> us smaller functions and more accurate computations for additional
> inlining.
>
> But that was not enough to ensure all accesses were properly
> initialized. ?Temporaries created after the referenced_vars gimple pass
> are not recorded in referenced_vars, so the resetting of the used flag
> in remove_unused_vars doesn't apply to them. ?I considered going over
> LOCAL_DECLS, but instead I decided to arrange for variables to be added
> to referenced_vars as they are created. ?I hope that's not just ok, but
> also desirable.
>
> With these fixes, in the second patch, I could get a full bootstrap and
> library build on x86_64-linux-gnu, fixing both PR 47106 bug and the
> regression it caused.
>
>
> The third patch introduces an abstract interface to set and clear the
> used flag, so that introducing the checks could be done with a simpler
> patch, and the fourth patch introduces the rejection of uses before
> initialization of this flag using a 3-state variable rather than a
> boolean, and additional checks to verify that the flags are at the
> expected states before we start computing what locals are unused. ?I
> don't expect these two patches to be installed, and I'm only posting
> them for the record, but if you think they'd useful (say, the abstract
> interface is desirable, or the additional checks should be enabled given
> some compile-time --enable-checking flag), let me know and I'll prepare
> and submit the patches for inclusion.
>
>
> I'm regstrapping them all together on x86_64-linux-gnu and
> i686-linux-gnu, at -O1 and -O3 in addition to the regular -O2 bootstrap
> (with -fcompare-debug at stage3, just because), and then I'll regstrap
> only the first two. ?This will take some time, because of the hardware
> failures I experienced, but I wanted to post the patches early so that,
> if they require changes or aren't acceptable, I can save some
> no-longer-copious machine time and devote it to the other bugs that are
> on my plate. ?Thanks for your understanding.
>
> Ok to install the first 2 if they pass regstrap? ?Opinions on the other 2?
>
>
> 1st: fix duplicate LOCAL_DECLs for inlined result decls
Ok (I think the assert is superfluous, a lot of things would already be
broken if that didn't hold ...). A further patch might simply pass
a struct function to add_local_decl instead.
> 2nd: fix bug and regression: remove unused vars after inlining and add
> newly-created variables to referenced_vars
Err, you add remove-unused-vars in function versioning. Which means
if that is useful we copied a function with unused locals - that's the place
this should (eventually) be fixed, not this one.
The 2nd part of the patch is ok (the gimple-low.c change).
>
>
> 3rd: introduce abstract interface to clear and test the used flag
+/* Clear VAR's used flag, so that it may be discarded during rtl
+ expansion. */
+
+static inline void
+clear_is_used (tree var)
I think this should just say "Clear VAR's used flag.". If it is just used
during RTL expansion that pass should simply compute it and
keep a local array/bitmap, not using var_ann (which really should go away).
That said, the patch is ok.
>
>
> 4th: check that used flag is never accessed before initialization, and
> that it's uninitialized after inlining and cleared after clearing it for
> all referenced_vars
This is not ok (at this stage anyway). I think we should compute
"used" where we need it (like remove-unused-vars does). If expand
needs it we should compute it there (I think there are no other users).
Richard.