On April 7, 2020 6:25:26 PM GMT+02:00, Martin Jambor <mjam...@suse.cz> wrote: >Hi, > >On Tue, Apr 07 2020, Richard Biener wrote: >> On Tue, 7 Apr 2020, Martin Jambor wrote: >> >>> Hi, >>> >>> when sra_modify_expr is invoked on an expression that modifies only >>> part of the underlying replacement, such as a BIT_FIELD_REF on a LHS >>> of an assignment and the SRA replacement's type is not compatible >with >>> what is being replaced (0th operand of the B_F_R in the above >>> example), it does not work properly, basically throwing away the >partd >>> of the expr that should have stayed intact. >>> >>> This is fixed in two ways. For BIT_FIELD_REFs, which operate on the >>> binary image of the replacement (and so in a way serve as a >>> VIEW_CONVERT_EXPR) we just do not bother with convertsing. For >>> REALPART_EXPRs and IMAGPART_EXPRs, we insert a VIEW_CONVERT_EXPR >under >>> the complex partial access expression, which is OK even in a LHS of >an >>> assignment (and other write contexts) because in those cases the >>> replacement is not going to be a giple register anyway. >>> >>> The testcase for handling REALPART_EXPR and IMAGPART_EXPR is a bit >>> fragile because SRA prefers complex and vector types over anything >else >>> (and in between the two it decides based on TYPE_UID which in my >testing >>> today always preferred complex types) and even when GIMPLE is wrong >I >>> often still did not get failing runs, so I only run it at -O1 (which >is >>> the only level where the the test fails for me). >>> >>> Bootstrapped and tested on x86_64-linux, bootstrap and testing on >>> i686-linux and aarch64-linux underway. >>> >>> OK for trunk (and subsequently for release branches) if it passes? >> >> OK. > >I must have been already half asleep when writing that it passed >testing. It did not, testsuite/gcc.c-torture/compile/pr42196-3.c fails >even on x86_64, because fwprop happily combines > > u$ci_10 = MEM[(union U *)&u]; > >and > > f1_6 = IMAGPART_EXPR <VIEW_CONVERT_EXPR<complex float>(u$ci_10)>; > >into > > f1_6 = IMAGPART_EXPR <MEM[(union U *)&u]>; > >and the gimple verifier of course does not like that. I'll have a look >at that tomorrow.
Ah, that might be a genuine fwprop bug. >> >> generate_subtree_copies contains similar code and the call in >> sra_modify_expr suggests that an (outer?) bit-field-ref is possible >> here, so is generate_subtree_copies affected as well? I'm not sure >> how subtree copy generation "works" when we already replaced sth > >Because they were just horribly problematic, SRA does not create >replacements for any scalar access that has a scalar ancestor or any >children in the access tree. So the generate_subtree_copies call is >only invoked when the expr itself is not replaced (because there are >children), even after SRA the expression will still load/store the >original bit of the original aggregate. The task of >generate_subtree_copies is to write all children replacements back to >the aggregate before it is read or re-load the replacements after a >write to the aggregate. So no data should be forgotten here. > >Martin
