From patchwork Sat Nov 3 11:53:29 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 196828 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 358B92C00B9 for ; Sat, 3 Nov 2012 22:53:37 +1100 (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=1352548418; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Received:From:To:Mail-Followup-To:Subject:Date:Message-ID: User-Agent:MIME-Version:Content-Type:Mailing-List:Precedence: List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=ytOXL1+tbXZe3Y6lQ0yAlUhD4ZI=; b=S6mwDGgkw9KSMUn PaaEWGVZJJTcltCBMGPG0mxZcludrkx1AtAgLC21JJsivnyFoCvowehC1H0IULRA MO9uElUfl1z4dm0db3cHLqsiq5hPXP6fVZEC4TU6SaqUYUAkL0Jr/hevPbnNVA79 LpGcxnN5QyKjVJbHdYylYo8m9/cc= 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:Received:Received:From:To:Mail-Followup-To:Subject:Date:Message-ID:User-Agent:MIME-Version:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=W7YYBCahC86CwZL2bS95PWMkAI4iuSIAKZT8nZ9A24YTh+npV0K5puPV7xrjkH sWlYM/YEYw8OWhGMJHhSm9DFZ6uxJuCSYvVyEC4OvQzjXnKWq2j8xWMGc2Tcw6Zc UfFWiVj0KAHlwYXwM8L+nzoKBzpJTxmsGOb1YtC6/wcxQ=; Received: (qmail 31257 invoked by alias); 3 Nov 2012 11:53:34 -0000 Received: (qmail 31247 invoked by uid 22791); 3 Nov 2012 11:53:34 -0000 X-SWARE-Spam-Status: No, hits=-3.6 required=5.0 tests=AWL, BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, DKIM_VALID, FREEMAIL_FROM, KHOP_RCVD_TRUST, KHOP_SPAMHAUS_DROP, NML_ADSP_CUSTOM_MED, RCVD_IN_DNSWL_LOW, RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-wi0-f173.google.com (HELO mail-wi0-f173.google.com) (209.85.212.173) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 03 Nov 2012 11:53:28 +0000 Received: by mail-wi0-f173.google.com with SMTP id hm4so1561093wib.8 for ; Sat, 03 Nov 2012 04:53:27 -0700 (PDT) Received: by 10.216.214.101 with SMTP id b79mr1629607wep.1.1351943607541; Sat, 03 Nov 2012 04:53:27 -0700 (PDT) Received: from localhost ([2.26.192.222]) by mx.google.com with ESMTPS id ea9sm2404726wib.11.2012.11.03.04.53.23 (version=TLSv1/SSLv3 cipher=OTHER); Sat, 03 Nov 2012 04:53:26 -0700 (PDT) From: Richard Sandiford To: gcc-patches@gcc.gnu.org Mail-Followup-To: gcc-patches@gcc.gnu.org, rdsandiford@googlemail.com Subject: RFC/A: set_mem_attributes_minus_bitpos tweak Date: Sat, 03 Nov 2012 11:53:29 +0000 Message-ID: <87hap6ewcm.fsf@talisman.home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 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 While working on the insertion and extraction optabs, I came across some strange-looking behaviour relatd to set_mem_attributes_minus_bitpos. In gcc.c-torture/execute/20030714-1.c we have: struct RenderStyle { struct NonInheritedFlags { union { struct { unsigned int _display : 4; unsigned int _bg_repeat : 2; bool _bg_attachment : 1; unsigned int _overflow : 4 ; unsigned int _vertical_align : 4; unsigned int _clear : 2; EPosition _position : 2; EFloat _floating : 2; unsigned int _table_layout : 1; bool _flowAroundFloats :1; unsigned int _styleType : 3; bool _hasHover : 1; bool _hasActive : 1; bool _clipSpecified : 1; unsigned int _unicodeBidi : 2; int _unused : 1; } f; int _niflags; }; } noninherited_flags; }; ... RenderStyle g__style; ... g__style.noninherited_flags.f._position = FIXED; ... expand_assignment calls: if (MEM_P (to_rtx)) { /* If the field is at offset zero, we could have been given the DECL_RTX of the parent struct. Don't munge it. */ to_rtx = shallow_copy_rtx (to_rtx); set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos); ... } where "to_rtx" is: (mem/c:SI (symbol_ref:DI ("g__style") [flags 0x4] ) [0 g__style+0 S4 A64]) and "to" is the component_ref. So far so good. But set_mem_attributes_minus_bitpos has: /* Default values from pre-existing memory attributes if present. */ refattrs = MEM_ATTRS (ref); if (refattrs) { /* ??? Can this ever happen? Calling this routine on a MEM that already carries memory attributes should probably be invalid. */ attrs.expr = refattrs->expr; attrs.offset_known_p = refattrs->offset_known_p; attrs.offset = refattrs->offset; attrs.size_known_p = refattrs->size_known_p; attrs.size = refattrs->size; attrs.align = refattrs->align; } which of course applies in this case: we have a MEM for g__style. I would expect many calls from this site are for MEMs with an existing MEM_EXPR. So the new attributes also start out describing g__style. Then we have: /* If the size is known, we can set that. */ if (TYPE_SIZE_UNIT (type) && host_integerp (TYPE_SIZE_UNIT (type), 1)) { attrs.size_known_p = true; attrs.size = tree_low_cst (TYPE_SIZE_UNIT (type), 1); } which sets the size to 1 byte (because that's all the bitfield occupies). Then later: /* If this is a field reference and not a bit-field, record it. */ /* ??? There is some information that can be gleaned from bit-fields, such as the word offset in the structure that might be modified. But skip it for now. */ else if (TREE_CODE (t) == COMPONENT_REF && ! DECL_BIT_FIELD (TREE_OPERAND (t, 1))) so we leave the offset and expr alone. The end result is: (mem/j/c:SI (symbol_ref:DI ("g__style") [flags 0x4] ) [0 g__style+0 S1 A64]) an SImode reference to the first byte (and only the first byte) of g__style. Then, when we apply adjust_bitfield_address, it looks like we're moving past the end of the region and so we drop the MEM_EXPR. In cases where set_mem_attributes_minus_bitpos does set MEM_EXPR based on the new tree, it also adds the bitpos to the size. But I think we should do that whenever we set the size based on the new tree, regardless of whether we were able to record a MEM_EXPR too. That said, this code has lots of ???s in it, so I'm not exactly confident about this change. Thoughts? Richard gcc/ * emit-rtl.c (set_mem_attributes_minus_bitpos): If setting the size based on the new tree, always include the bitpos in it. Index: gcc/emit-rtl.c =================================================================== --- gcc/emit-rtl.c 2012-11-01 20:43:37.193362850 +0000 +++ gcc/emit-rtl.c 2012-11-01 20:44:22.248362740 +0000 @@ -1569,7 +1569,7 @@ get_mem_align_offset (rtx mem, unsigned set_mem_attributes_minus_bitpos (rtx ref, tree t, int objectp, HOST_WIDE_INT bitpos) { - HOST_WIDE_INT apply_bitpos = 0; + tree expr = NULL_TREE; tree type; struct mem_attrs attrs, *defattrs, *refattrs; addr_space_t as; @@ -1679,11 +1679,7 @@ set_mem_attributes_minus_bitpos (rtx ref attrs.align = MAX (attrs.align, TYPE_ALIGN (type)); /* If the size is known, we can set that. */ - if (TYPE_SIZE_UNIT (type) && host_integerp (TYPE_SIZE_UNIT (type), 1)) - { - attrs.size_known_p = true; - attrs.size = tree_low_cst (TYPE_SIZE_UNIT (type), 1); - } + tree new_size = TYPE_SIZE_UNIT (type); /* If T is not a type, we may be able to deduce some more information about the expression. */ @@ -1738,17 +1734,10 @@ set_mem_attributes_minus_bitpos (rtx ref /* If this is a decl, set the attributes of the MEM from it. */ if (DECL_P (t)) { - attrs.expr = t; + expr = t; attrs.offset_known_p = true; attrs.offset = 0; - apply_bitpos = bitpos; - if (DECL_SIZE_UNIT (t) && host_integerp (DECL_SIZE_UNIT (t), 1)) - { - attrs.size_known_p = true; - attrs.size = tree_low_cst (DECL_SIZE_UNIT (t), 1); - } - else - attrs.size_known_p = false; + new_size = DECL_SIZE_UNIT (t); attrs.align = DECL_ALIGN (t); align_computed = true; } @@ -1770,10 +1759,9 @@ set_mem_attributes_minus_bitpos (rtx ref else if (TREE_CODE (t) == COMPONENT_REF && ! DECL_BIT_FIELD (TREE_OPERAND (t, 1))) { - attrs.expr = t; + expr = t; attrs.offset_known_p = true; attrs.offset = 0; - apply_bitpos = bitpos; /* ??? Any reason the field size would be different than the size we got from the type? */ } @@ -1812,7 +1800,7 @@ set_mem_attributes_minus_bitpos (rtx ref if (DECL_P (t2)) { - attrs.expr = t2; + expr = t2; attrs.offset_known_p = false; if (host_integerp (off_tree, 1)) { @@ -1824,18 +1812,16 @@ set_mem_attributes_minus_bitpos (rtx ref align_computed = true; attrs.offset_known_p = true; attrs.offset = ioff; - apply_bitpos = bitpos; } } else if (TREE_CODE (t2) == COMPONENT_REF) { - attrs.expr = t2; + expr = t2; attrs.offset_known_p = false; if (host_integerp (off_tree, 1)) { attrs.offset_known_p = true; attrs.offset = tree_low_cst (off_tree, 1); - apply_bitpos = bitpos; } /* ??? Any reason the field size would be different than the size we got from the type? */ @@ -1846,10 +1832,9 @@ set_mem_attributes_minus_bitpos (rtx ref else if (TREE_CODE (t) == MEM_REF || TREE_CODE (t) == TARGET_MEM_REF) { - attrs.expr = t; + expr = t; attrs.offset_known_p = true; attrs.offset = 0; - apply_bitpos = bitpos; } if (!align_computed) @@ -1861,17 +1846,22 @@ set_mem_attributes_minus_bitpos (rtx ref else as = TYPE_ADDR_SPACE (type); - /* If we modified OFFSET based on T, then subtract the outstanding - bit position offset. Similarly, increase the size of the accessed - object to contain the negative offset. */ - if (apply_bitpos) + if (expr) { - gcc_assert (attrs.offset_known_p); - attrs.offset -= apply_bitpos / BITS_PER_UNIT; - if (attrs.size_known_p) - attrs.size += apply_bitpos / BITS_PER_UNIT; + attrs.expr = expr; + /* If we modified OFFSET based on T, then subtract the outstanding + bit position offset. */ + if (attrs.offset_known_p) + attrs.offset -= bitpos / BITS_PER_UNIT; } - + if (host_integerp (new_size, 1)) + { + attrs.size_known_p = true; + /* Include the outstanding bit offset in the size, so that the + initial reference includes the leading gap and T. */ + attrs.size = tree_low_cst (new_size, 1) + bitpos / BITS_PER_UNIT; + } + /* Now set the attributes we computed above. */ attrs.addrspace = as; set_mem_attrs (ref, &attrs);