On Fri, Jun 1, 2012 at 1:44 PM, Martin Jambor <mjam...@suse.cz> wrote: > Hi, > > On Fri, Jun 01, 2012 at 11:31:20AM +0200, Richard Guenther wrote: >> On Fri, Jun 1, 2012 at 6:02 AM, Andrew Pinski >> <andrew.pin...@caviumnetworks.com> wrote: >> > Hi, >> > When I modified GCC to change the majority of bitfield accesses >> > which were done via component ref to BIT_FIELD_REF, SRA messes up >> > because when it does the replacement it still tries to use the >> > BIT_FIELD_REF except it never places the old value in the struct for >> > the BIT_FIELD_REF to work correctly. >> > >> > This patch fixes the problem by expanding what BIT_FIELD_REF does >> > internally for both writing and reading. Note we can't use >> > BIT_FIELD_REF directly on the left hand since we don't support partial >> > writes yet (except for vector and complex types). >> > >> > This is only a RFC since I don't know a way to reproduce this bug on >> > the trunk. I tested it on x86_64-linux-gnu with no regressions. >> >> I'd rather disqualify SRA of BIT_FIELD_REFs on the LHS for now. My goal >> was to enable SRA of bitfields using the DECL_BIT_FIELD_REPRESENTATIVE >> work - something you go against with replacing them with BIT_FIELD_REFs. > > SRA of bit-fields works now, it is just rather simple and can't > optimize the bit-field accesses as we probably should. Therefore I am > all for using DECL_BIT_FIELD_REPRESENTATIVEs and looked at those > patches with interest, I'm just wondering why we'd want to do it for > non-addressable structures only, which is something SRA would imply if > this "lowering" was part of it.
SRA would not be the canonical lowering, it would just use the same lowering way until we implement a generic lowering. Given that when there is an SRA opportunity the lowering is certainly profitable. > BIT_FIELD_REFs of non-vectors on the LHS are not much tested, I'm > afraid. I it is quite possible it does not do the right thing. > Nevertheless, making regions accessed through them unscalarizable > might also be an option, if it does not induce significant penalties > anywhere. Indeed. I'd prefer that for now. Btw, Andrew, your second patch doesn't look sensible to me, at least without comments or a testcase to inspect. Richard. > Thanks, > > Martin > >> >> You'd replace >> >> x = a.b; >> >> with >> >> tem = a.<representative for b>; >> x = BIT_FIELD_REF <tem, ....>; >> >> and for stores with a read-modify-write sequence, possibly adding >> the bitfield-insert tree I proposed some time ago. Replace >> >> a.b = x; >> >> with >> >> tem = a.<representative for b> >> tem = BIT_FIELD_EXPR <tem, x, ...>; >> a.<representative for b> = tem; >> >> and I'd do this replacement in SRA for now whenever it would decide to >> SRA a bitfield. >> >> Richard. >> >> > Thanks, >> > Andrew Pinski >> > >> > ChangeLog: >> > * tree-sra.c (sra_modify_expr): Handle BIT_FIELD_REF specially if we >> > are doing a replacement of the struct with one variable.
