From patchwork Sun Oct 24 03:12:46 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jie Zhang X-Patchwork-Id: 69023 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 60EC1B70A5 for ; Sun, 24 Oct 2010 14:13:01 +1100 (EST) Received: (qmail 22886 invoked by alias); 24 Oct 2010 03:12:59 -0000 Received: (qmail 22874 invoked by uid 22791); 24 Oct 2010 03:12:58 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL, BAYES_00, TW_DQ, TW_DR, T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (38.113.113.100) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 24 Oct 2010 03:12:54 +0000 Received: (qmail 25362 invoked from network); 24 Oct 2010 03:12:52 -0000 Received: from unknown (HELO ?192.168.1.106?) (jie@127.0.0.2) by mail.codesourcery.com with ESMTPA; 24 Oct 2010 03:12:52 -0000 Message-ID: <4CC3A42E.3000405@codesourcery.com> Date: Sun, 24 Oct 2010 11:12:46 +0800 From: Jie Zhang User-Agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.9) Gecko/20100918 Icedove/3.1.4 MIME-Version: 1.0 To: Richard Guenther CC: gcc-patches@gcc.gnu.org, Richard Earnshaw , Paul Brook , nickc@redhat.com Subject: Re: PING: Default to -fstrict-volatile-bitfields for ARM EABI References: <4CC0EA53.4070100@codesourcery.com> <4CC1D201.7000906@codesourcery.com> In-Reply-To: <4CC1D201.7000906@codesourcery.com> 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 10/23/2010 02:03 AM, Jie Zhang wrote: > On 10/22/2010 05:40 PM, Richard Guenther wrote: >> On Fri, 22 Oct 2010, Jie Zhang wrote: >> >>> Since the regression has been fixed now, it's time to reconsider this >>> patch: >>> >>> http://gcc.gnu.org/ml/gcc-patches/2010-09/msg00155.html >>> >>> Regression tested on latest trunk for arm-none-eabi with c,lto,c++. >>> >>> ARM maintainers: is the ARM port part OK? >>> >>> Richard G: Could you help me review the stor-layout.c part? >> >> Why is the stor-layout.c part necessary? Why does it not possibly >> cause layout changes? I'm not very familiar with the stor-layout.c >> code, and given that questions I think I'm not the right person >> to review this patch. > > Thanks for your questions! It's not necessary indeed. I just retested > the patch with the stor-layout.c change dropped. It has no regressions > on arm-none-eabi with c,c++,lto and it passes all three new tests. I > don't remember the exact details why I changed stor-layout.c at that > time. So now the patch is only for ARM port. The updated patch is attached. > I have been trying to recall the reason of the stor-layout.c change for the last two days. Now I know. The regular expressions used in the three new new tests have a bug. The "." matches "\n" in expect, so "ldr\[\\t \]+.*,\[\\t \]*\\\[.*\\\]" will match ldr r3, .L2 ldrb r0, [r3, #1] @ zero_extendqisi2 but it's intended to match one instruction like ldr r0, [r3, #0] So the PASSes are fake after dropping the stor-layout.c change. Now I can explain the stor-layout.c change. When layout_decl processes "b" bit-field in typedef struct { volatile unsigned long a:8; volatile unsigned long b:8; volatile unsigned long c:16; } BitStruct; , this piece of code /* See if we can use an ordinary integer mode for a bit-field. Conditions are: a fixed size that is correct for another mode, occupying a complete byte or bytes on proper boundary, and not volatile or not -fstrict-volatile-bitfields. */ if (TYPE_SIZE (type) != 0 && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT) { enum machine_mode xmode = mode_for_size_tree (DECL_SIZE (decl), MODE_INT, 1); unsigned int xalign = GET_MODE_ALIGNMENT (xmode); if (xmode != BLKmode && !(xalign > BITS_PER_UNIT && DECL_PACKED (decl)) && (known_align == 0 || known_align >= xalign)) { DECL_ALIGN (decl) = MAX (xalign, DECL_ALIGN (decl)); DECL_MODE (decl) = xmode; DECL_BIT_FIELD (decl) = 0; } } will change the mode of "b" to QImode and clear DECL_BIT_FIELD (decl). Then later access to "b" will use byte load instruction instead of word load instruction as required by -fstrict-volatile-bitfields. The attached patch fixes the tests and is resubmitted for approval for stor-layout.c change. Is it OK? Regards, * stor-layout.c (layout_decl): Use the field's type to determine the mode and keep DECL_BIT_FIELD for a volatile bit-field. * config/arm/arm.c (arm_override_options): Default to -fstrict-volatile-bitfields. testsuite/ * gcc.target/arm/volatile-bitfields-1.c: New test. * gcc.target/arm/volatile-bitfields-2.c: New test. * gcc.target/arm/volatile-bitfields-3.c: New test. Index: testsuite/gcc.target/arm/volatile-bitfields-1.c =================================================================== --- testsuite/gcc.target/arm/volatile-bitfields-1.c (revision 0) +++ testsuite/gcc.target/arm/volatile-bitfields-1.c (revision 0) @@ -0,0 +1,18 @@ +/* { dg-require-effective-target arm_eabi } */ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +typedef struct { + char a:1; + char b:7; + int c; +} BitStruct; + +volatile BitStruct bits; + +int foo () +{ + return bits.b; +} + +/* { dg-final { scan-assembler "ldrb\[\\t \]+\[^\n\]*,\[\\t \]*\\\[\[^\n\]*\\\]" } } */ Index: testsuite/gcc.target/arm/volatile-bitfields-2.c =================================================================== --- testsuite/gcc.target/arm/volatile-bitfields-2.c (revision 0) +++ testsuite/gcc.target/arm/volatile-bitfields-2.c (revision 0) @@ -0,0 +1,18 @@ +/* { dg-require-effective-target arm_eabi } */ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +typedef struct { + volatile unsigned long a:8; + volatile unsigned long b:8; + volatile unsigned long c:16; +} BitStruct; + +BitStruct bits; + +unsigned long foo () +{ + return bits.b; +} + +/* { dg-final { scan-assembler "ldr\[\\t \]+\[^\n\]*,\[\\t \]*\\\[\[^\n\]*\\\]" } } */ Index: testsuite/gcc.target/arm/volatile-bitfields-3.c =================================================================== --- testsuite/gcc.target/arm/volatile-bitfields-3.c (revision 0) +++ testsuite/gcc.target/arm/volatile-bitfields-3.c (revision 0) @@ -0,0 +1,18 @@ +/* { dg-require-effective-target arm_eabi } */ +/* { dg-do compile } */ +/* { dg-options "-O2" } */ + +typedef struct { + volatile unsigned long a:8; + volatile unsigned long b:8; + volatile unsigned long c:16; +} BitStruct; + +BitStruct bits; + +unsigned long foo () +{ + return bits.c; +} + +/* { dg-final { scan-assembler "ldr\[\\t \]+\[^\n\]*,\[\\t \]*\\\[\[^\n\]*\\\]" } } */ Index: stor-layout.c =================================================================== --- stor-layout.c (revision 165879) +++ stor-layout.c (working copy) @@ -661,11 +661,14 @@ layout_decl (tree decl, unsigned int kno } /* See if we can use an ordinary integer mode for a bit-field. - Conditions are: a fixed size that is correct for another mode - and occupying a complete byte or bytes on proper boundary. */ + Conditions are: a fixed size that is correct for another mode, + occupying a complete byte or bytes on proper boundary, + and not volatile or not -fstrict-volatile-bitfields. */ if (TYPE_SIZE (type) != 0 && TREE_CODE (TYPE_SIZE (type)) == INTEGER_CST - && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT) + && GET_MODE_CLASS (TYPE_MODE (type)) == MODE_INT + && !(TREE_THIS_VOLATILE (decl) + && flag_strict_volatile_bitfields > 0)) { enum machine_mode xmode = mode_for_size_tree (DECL_SIZE (decl), MODE_INT, 1); Index: config/arm/arm.c =================================================================== --- config/arm/arm.c (revision 165879) +++ config/arm/arm.c (working copy) @@ -1971,6 +1971,10 @@ arm_option_override (void) global_options.x_param_values, global_options_set.x_param_values); + /* ARM EABI defaults to strict volatile bitfields. */ + if (TARGET_AAPCS_BASED && flag_strict_volatile_bitfields < 0) + flag_strict_volatile_bitfields = 1; + /* Register global variables with the garbage collector. */ arm_add_gc_roots (); }