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. > > 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
