From patchwork Sat Jun 2 09:02:55 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Andrew Pinski X-Patchwork-Id: 162405 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 62FD0B700C for ; Sat, 2 Jun 2012 19:03:34 +1000 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1339232615; h=Comment: DomainKey-Signature:Received:Received:Received:Received: MIME-Version:Received:Received:In-Reply-To:References:Date: Message-ID:Subject:From:To:Content-Type:Mailing-List:Precedence: List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=2RqUIa3cfqgZ01CDzbVwqJ5KUY4=; b=haUuVNYV+hTebUk GvXFiituh7L/Oty9bJueRByiIT6HUMoryVLCFFKLrtfSjGQDoEJiXsW+xESwBo/B 4HM5vaPPb2XTyyinLUP9/3iIhjpoUambNek/vP/TlcOKhscmLvThUF92YlZzBkCF cnGFA7FVlWXKtc2TMkel5cZ2Mm0s= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:MIME-Version:Received:Received:In-Reply-To:References:Date:Message-ID:Subject:From:To:Content-Type:X-IsSubscribed:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=j2dRmKBUMb510BxKdiSbbyvWCCGEfIoS/cZmXW3dZIcB+WQMmDQO18jhtAqU9J n2WunmlIU82GPsfz/5WYhG2SuhmVDEB23vm22TIj5CzuhYGUIgZYibIqScS577S1 W3tsGAfCehihI9iifUFWSH2/UQAxl/cepUWjrF1HOwYSU=; Received: (qmail 23507 invoked by alias); 2 Jun 2012 09:03:30 -0000 Received: (qmail 23498 invoked by uid 22791); 2 Jun 2012 09:03:28 -0000 X-SWARE-Spam-Status: No, hits=-5.0 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, FREEMAIL_FROM, KHOP_RCVD_TRUST, KHOP_THREADED, RCVD_IN_DNSWL_LOW, RCVD_IN_HOSTKARMA_YE, TW_TM X-Spam-Check-By: sourceware.org Received: from mail-wi0-f169.google.com (HELO mail-wi0-f169.google.com) (209.85.212.169) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 02 Jun 2012 09:02:57 +0000 Received: by wibhn14 with SMTP id hn14so1195714wib.2 for ; Sat, 02 Jun 2012 02:02:55 -0700 (PDT) MIME-Version: 1.0 Received: by 10.216.218.216 with SMTP id k66mr4402451wep.191.1338627775634; Sat, 02 Jun 2012 02:02:55 -0700 (PDT) Received: by 10.216.38.132 with HTTP; Sat, 2 Jun 2012 02:02:55 -0700 (PDT) In-Reply-To: <20120601114451.GA26766@virgil.arch.suse.de> References: <20120601114451.GA26766@virgil.arch.suse.de> Date: Sat, 2 Jun 2012 02:02:55 -0700 Message-ID: Subject: Re: [RFC] Fix SRA with respect of BIT_FIELD_REF From: Andrew Pinski To: Richard Guenther , Andrew Pinski , GCC Patches X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org On Fri, Jun 1, 2012 at 4:44 AM, Martin Jambor 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 >> 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. > > 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. Here is a new patch which fixes BFR's in a much simpler way. The problem is SRA recognizes it needs to reread the replacement but it forgets it needs to the write in the replacement right before the action happens. Thanks, Andrew pinski > > Thanks, > > Martin > >> >> You'd replace >> >>   x = a.b; >> >> with >> >>   tem = a.; >>   x = BIT_FIELD_REF ; >> >> 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. >>  tem = BIT_FIELD_EXPR ; >>  a. = 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. Index: tree-sra.c =================================================================== --- tree-sra.c (revision 188138) +++ tree-sra.c (working copy) @@ -2602,10 +2602,20 @@ sra_modify_expr (tree *expr, gimple_stmt if (!useless_type_conversion_p (type, access->type)) { tree ref; + gimple stmt; ref = build_ref_for_model (loc, access->base, access->offset, access, NULL, false); + /* Reads and writes need at least the original access setup. */ + if (access->grp_partial_lhs) + repl = force_gimple_operand_gsi (gsi, repl, true, NULL_TREE, + true, GSI_SAME_STMT); + stmt = gimple_build_assign (ref, repl); + gimple_set_location (stmt, loc); + gsi_insert_before (gsi, stmt, GSI_SAME_STMT); + + /* Writes read from the original access after the write has happened. */ if (write) { gimple stmt; @@ -2617,17 +2627,6 @@ sra_modify_expr (tree *expr, gimple_stmt gimple_set_location (stmt, loc); gsi_insert_after (gsi, stmt, GSI_NEW_STMT); } - else - { - gimple stmt; - - if (access->grp_partial_lhs) - repl = force_gimple_operand_gsi (gsi, repl, true, NULL_TREE, - true, GSI_SAME_STMT); - stmt = gimple_build_assign (ref, repl); - gimple_set_location (stmt, loc); - gsi_insert_before (gsi, stmt, GSI_SAME_STMT); - } } else *expr = repl;