Hi,
On Mon, Nov 03, 2014 at 05:17:22PM +0100, Marc Glisse wrote:
> On Mon, 3 Nov 2014, Martin Jambor wrote:
> >On Mon, Nov 03, 2014 at 01:59:24PM +0100, Marc Glisse wrote:
> >>
> >>now that the update_address_taken patch is in, let me re-post the
> >>SRA follow-up. With this patch, testcase pr60517.C (attached) has a
> >>use of an undefined variable at the time of the uninit pass. Sadly,
> >>while this warned with earlier versions of the other patch (when I
> >>was inserting default definitions of new variables), it does not
> >>anymore because we have TREE_NO_WARNING all over the place. Still, I
> >>believe this is a step in the right direction, and it passed
> >>bootstrap+testsuite (minus the new testcase).
> >
> >I am not sure what you mean by "minus the new testcase" but, with the
> >gimple checking compiler at least, the testcase produces an ICE:
> >
> > pr60517.C: In function ‘double foo(A)’:
> > pr60517.C:15:1: error: non-decl/MEM_REF LHS in clobber statement
> > }
> > ^
> > SR.1_8
> > SR.1_8 ={v} {CLOBBER};
> > pr60517.C:15:1: internal compiler error: verify_gimple failed
> > 0xc7810d verify_gimple_in_cfg(function*, bool)
> > /home/mjambor/gcc/mine/src/gcc/tree-cfg.c:5039
> > 0xb8256f execute_function_todo
> > /home/mjambor/gcc/mine/src/gcc/passes.c:1758
> > 0xb83183 execute_todo
> > /home/mjambor/gcc/mine/src/gcc/passes.c:1815
> >
> >The problem is exactly what the verifier complains about, a clobber of
> >an SSA_NAME which I agree with the verifier is a really bad idea.
> >
> >If you want SRA to produce information that at a given point a scalar
> >replacement (which will become an SSA_NAME) does not have any valid
> >data, the way to represent that is to load it with a value from a
> >default-definition SSA name. Something along the lines of
> >get_repl_default_def_ssa_name which unfortunately you cannot directly
> >use without confusing the SSA renamer.
> >
> >The easiest way, at least to test a proof of concept, is probably to
> >create another declaration of the same type (reusing bits from
> >create_access_replacement, especially setting the same
> >DECL_DEBUG_EXPR), get its default-definition using
> >get_or_create_ssa_default_def and then assign that to the replacement
> >instead of trying to clobber it.
> >
> >(A more correct approach would be to design a way of telling the
> >renamer that at this point the scalar becomes undefined and that it
> >should start using default-defs until the next definition appears but
> >that is likely to be difficult, although that is just my guess, it
> >might be easy.)
> >
> >>Would it be ok to commit the tree-sra.c change?
> >
> >I don't think so, sorry about the bad news.
>
> Thanks for your help. I am quite confused. I am not seeing the
> failure you mention, but since I am building from trunk with no
> specific option I should be getting gimple checking.
>
> The way I expect things to work:
> 1) we have a clobber for a variable V
> 2) SRA creates a variable SR and replaces uses of V
> 3) (new) next to V's clobber, we insert a clobber for SR
> 4) update_address_taken notices that SR does not have its address
> taken, and thus replaces it by SSA_NAMEs, and after the clobber by
> an undefined variable (yesterday it would just have removed the
> clobber).
update_address_taken? The conversion to SSA form is done by means of
returning TODO_update_ssa from the (early) SRA pass (which is what I
mean by the renamer).
>
> What you describe in a parenthesis about the renamer is pretty much
> what I think rewrite_update_stmt does, with the clobber as the hint.
>
> Do you have further comments, based on these explanations, that
> could help me pinpoint what is going wrong?
I just applied your patch on top of trunk revision 217032 on my
x86_64-linux (openSUSE 13.1) desktop, configured with
/home/mjambor/gcc/mine/src/configure --prefix=/home/mjambor/gcc/mine/inst
--enable-languages=c,c++ --enable-checking=yes --disable-bootstrap
--with-plugin-ld=/home/mjambor/binutils/obj/gold/ld-new --disable-libsanitizer
--disable-multilib
built with simple make -j8,
and ran the testcase with make -k check RUNTESTFLAGS="dg.exp=pr60517.C"
And the error popped out in gcc/testsuite/g++/g++.log. I really can't
think of reason why you don't see it.
But I do and little wonder, really, given the second condition in
verify_gimple_assign_single, which is there for more than a year.
Looking at the dumped statement it seems that the LHS is already an
SSA_NAME. Moreover, when I dumped the erroneous body in gdb, I have
not seen any default definitions anywhere. So if your intention was
to teach the renamer to turn this into default definitions, it does
not work and update_ssa did not get the hint.
Martin
