From patchwork Thu Oct 21 12:22:49 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jie Zhang X-Patchwork-Id: 68601 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 82A03B70EC for ; Thu, 21 Oct 2010 23:23:10 +1100 (EST) Received: (qmail 12398 invoked by alias); 21 Oct 2010 12:23:05 -0000 Received: (qmail 12386 invoked by uid 22791); 21 Oct 2010 12:23:01 -0000 X-SWARE-Spam-Status: No, hits=-1.9 required=5.0 tests=AWL, BAYES_00, 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; Thu, 21 Oct 2010 12:22:55 +0000 Received: (qmail 6306 invoked from network); 21 Oct 2010 12:22:52 -0000 Received: from unknown (HELO ?192.168.1.106?) (jie@127.0.0.2) by mail.codesourcery.com with ESMTPA; 21 Oct 2010 12:22:52 -0000 Message-ID: <4CC03099.3010803@codesourcery.com> Date: Thu, 21 Oct 2010 20:22:49 +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: Mark Mitchell CC: Eric Botcazou , Richard Henderson , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Prefer packed attribute when conflicts with volatile References: <4CB866EA.3000709@codesourcery.com> <4CBDBA32.30504@codesourcery.com> <201010201935.36445.ebotcazou@adacore.com> <4CBF9E5A.1040706@codesourcery.com> <4CBFC836.40905@codesourcery.com> In-Reply-To: <4CBFC836.40905@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 Hi Mark, Thanks for your review! On 10/21/2010 12:57 PM, Mark Mitchell wrote: > On 10/20/2010 6:58 PM, Jie Zhang wrote: > >> No problem. I can try someone else. The patch changes code in tree -> >> RTL. You are listed as the maintainer for RTL optimizers. I know it's >> not perfectly match but you and Richard Henderson are recommended for >> reviewing my patch on IRC. Maybe we should have a maintainer for expand >> pass? > > I think the patch makes sense, but I will pick a few nits. :-) > > Algorithmically, why do we have to call extract_fixed_bit_field twice > (once with final clear and once with it set)? Can't we issue the > warning and then just keep going? Or, in the worst case, do a goto to > the top of the function? Callers shouldn't have to handle this > final/not-final distinction. > extract_fixed_bit_field first calculates the best mode, then does works based on that mode. When it gets to the point that packed attribute conflicts with volatile, it need recalculate the best mode and restart works. So we cannot issue the warning and then just keep going. After more thought, now I think it should be enough to just call extract_split_bit_field for such cases. extract_split_bit_field will call into extract_fixed_bit_field. It has the same effect of restarting extract_fixed_bit_field. The resulted patch is simplified. > Also, as a point of style, new boolean parameters should be declared as > "bool", not as "int". So, for example, "extract_fixed_bit_field" should > have "packedp" as "bool". > Done. > (Also, although it's not specified as a standard GNU style, I like to write: > > ..., /*packedp=*/true, ... > > when passing cryptic boolean arguments to functions; otherwise, it's > confusing to look at: > > f (true, false, 1, 0, false) > > and work out what it's doing. You can decide whether you want to use > that style or not.) > That's good practice. But only there are very few occurrences in GCC. So I would just keep current code. > Please resubmit with those changes. > The new patch is attached. Test is still going. Is it OK if the test result is good? Regards, * expr.c (emit_group_load_1): Update calls to extract_bit_field. (copy_blkmode_from_reg): Likewise. (read_complex_part): Likewise. (expand_expr_real_1): Calculate packedp and pass it to extract_bit_field. * expr.h (extract_bit_field): Update declaration. * calls.c (store_unaligned_arguments_into_pseudos): Update call to extract_bit_field. * expmed.c (extract_fixed_bit_field): Update calls to extract_fixed_bit_field. (store_split_bit_field): Likewise. (extract_bit_field_1): Add new argument packedp. (extract_bit_field): Add new argument packedp. (extract_fixed_bit_field): Add new argument packedp and let packed attribute override volatile. * stmt.c (expand_return): Update call to extract_bit_field. Index: expr.c =================================================================== --- expr.c (revision 165712) +++ expr.c (working copy) @@ -1703,7 +1703,7 @@ emit_group_load_1 (rtx *tmps, rtx dst, r && (!REG_P (tmps[i]) || GET_MODE (tmps[i]) != mode)) tmps[i] = extract_bit_field (tmps[i], bytelen * BITS_PER_UNIT, (bytepos % slen0) * BITS_PER_UNIT, - 1, NULL_RTX, mode, mode); + 1, false, NULL_RTX, mode, mode); } else { @@ -1713,7 +1713,7 @@ emit_group_load_1 (rtx *tmps, rtx dst, r mem = assign_stack_temp (GET_MODE (src), slen, 0); emit_move_insn (mem, src); tmps[i] = extract_bit_field (mem, bytelen * BITS_PER_UNIT, - 0, 1, NULL_RTX, mode, mode); + 0, 1, false, NULL_RTX, mode, mode); } } /* FIXME: A SIMD parallel will eventually lead to a subreg of a @@ -1754,7 +1754,7 @@ emit_group_load_1 (rtx *tmps, rtx dst, r tmps[i] = src; else tmps[i] = extract_bit_field (src, bytelen * BITS_PER_UNIT, - bytepos * BITS_PER_UNIT, 1, NULL_RTX, + bytepos * BITS_PER_UNIT, 1, false, NULL_RTX, mode, mode); if (shift) @@ -2167,7 +2167,7 @@ copy_blkmode_from_reg (rtx tgtblk, rtx s bitpos for the destination store (left justified). */ store_bit_field (dst, bitsize, bitpos % BITS_PER_WORD, copy_mode, extract_bit_field (src, bitsize, - xbitpos % BITS_PER_WORD, 1, + xbitpos % BITS_PER_WORD, 1, false, NULL_RTX, copy_mode, copy_mode)); } @@ -2924,7 +2924,7 @@ read_complex_part (rtx cplx, bool imag_p } return extract_bit_field (cplx, ibitsize, imag_p ? ibitsize : 0, - true, NULL_RTX, imode, imode); + true, false, NULL_RTX, imode, imode); } /* A subroutine of emit_move_insn_1. Yet another lowpart generator. @@ -8938,6 +8938,7 @@ expand_expr_real_1 (tree exp, rtx target HOST_WIDE_INT bitsize, bitpos; tree offset; int volatilep = 0, must_force_mem; + bool packedp = false; tree tem = get_inner_reference (exp, &bitsize, &bitpos, &offset, &mode1, &unsignedp, &volatilep, true); rtx orig_op0, memloc; @@ -8947,6 +8948,11 @@ expand_expr_real_1 (tree exp, rtx target infinitely recurse. */ gcc_assert (tem != exp); + if (TYPE_PACKED (TREE_TYPE (TREE_OPERAND (exp, 0))) + || (TREE_CODE (TREE_OPERAND (exp, 1)) == FIELD_DECL + && DECL_PACKED (TREE_OPERAND (exp, 1)))) + packedp = true; + /* If TEM's type is a union of variable size, pass TARGET to the inner computation, since it will need a temporary and TARGET is known to have to do. This occurs in unchecked conversion in Ada. */ @@ -9159,7 +9165,7 @@ expand_expr_real_1 (tree exp, rtx target if (MEM_P (op0) && REG_P (XEXP (op0, 0))) mark_reg_pointer (XEXP (op0, 0), MEM_ALIGN (op0)); - op0 = extract_bit_field (op0, bitsize, bitpos, unsignedp, + op0 = extract_bit_field (op0, bitsize, bitpos, unsignedp, packedp, (modifier == EXPAND_STACK_PARM ? NULL_RTX : target), ext_mode, ext_mode); Index: expr.h =================================================================== --- expr.h (revision 165712) +++ expr.h (working copy) @@ -668,7 +668,7 @@ mode_for_extraction (enum extraction_pat extern void store_bit_field (rtx, unsigned HOST_WIDE_INT, unsigned HOST_WIDE_INT, enum machine_mode, rtx); extern rtx extract_bit_field (rtx, unsigned HOST_WIDE_INT, - unsigned HOST_WIDE_INT, int, rtx, + unsigned HOST_WIDE_INT, int, bool, rtx, enum machine_mode, enum machine_mode); extern rtx extract_low_bits (enum machine_mode, enum machine_mode, rtx); extern rtx expand_mult (enum machine_mode, rtx, rtx, rtx, int); Index: calls.c =================================================================== --- calls.c (revision 165712) +++ calls.c (working copy) @@ -886,7 +886,7 @@ store_unaligned_arguments_into_pseudos ( int bitsize = MIN (bytes * BITS_PER_UNIT, BITS_PER_WORD); args[i].aligned_regs[j] = reg; - word = extract_bit_field (word, bitsize, 0, 1, NULL_RTX, + word = extract_bit_field (word, bitsize, 0, 1, false, NULL_RTX, word_mode, word_mode); /* There is no need to restrict this code to loading items Index: expmed.c =================================================================== --- expmed.c (revision 165712) +++ expmed.c (working copy) @@ -53,7 +53,7 @@ static void store_split_bit_field (rtx, static rtx extract_fixed_bit_field (enum machine_mode, rtx, unsigned HOST_WIDE_INT, unsigned HOST_WIDE_INT, - unsigned HOST_WIDE_INT, rtx, int); + unsigned HOST_WIDE_INT, rtx, int, bool); static rtx mask_rtx (enum machine_mode, int, int, int); static rtx lshift_value (enum machine_mode, rtx, int, int); static rtx extract_split_bit_field (rtx, unsigned HOST_WIDE_INT, @@ -1083,7 +1083,7 @@ store_split_bit_field (rtx op0, unsigned endianness compensation) to fetch the piece we want. */ part = extract_fixed_bit_field (word_mode, value, 0, thissize, total_bits - bitsize + bitsdone, - NULL_RTX, 1); + NULL_RTX, 1, false); } else { @@ -1094,7 +1094,7 @@ store_split_bit_field (rtx op0, unsigned & (((HOST_WIDE_INT) 1 << thissize) - 1)); else part = extract_fixed_bit_field (word_mode, value, 0, thissize, - bitsdone, NULL_RTX, 1); + bitsdone, NULL_RTX, 1, false); } /* If OP0 is a register, then handle OFFSET here. @@ -1160,7 +1160,8 @@ convert_extracted_bit_field (rtx x, enum static rtx extract_bit_field_1 (rtx str_rtx, unsigned HOST_WIDE_INT bitsize, - unsigned HOST_WIDE_INT bitnum, int unsignedp, rtx target, + unsigned HOST_WIDE_INT bitnum, + int unsignedp, bool packedp, rtx target, enum machine_mode mode, enum machine_mode tmode, bool fallback_p) { @@ -1441,7 +1442,7 @@ extract_bit_field_1 (rtx str_rtx, unsign rtx result_part = extract_bit_field (op0, MIN (BITS_PER_WORD, bitsize - i * BITS_PER_WORD), - bitnum + bit_offset, 1, target_part, mode, + bitnum + bit_offset, 1, false, target_part, mode, word_mode); gcc_assert (target_part); @@ -1640,7 +1641,7 @@ extract_bit_field_1 (rtx str_rtx, unsign xop0 = adjust_address (op0, bestmode, xoffset); xop0 = force_reg (bestmode, xop0); result = extract_bit_field_1 (xop0, bitsize, xbitpos, - unsignedp, target, + unsignedp, packedp, target, mode, tmode, false); if (result) return result; @@ -1654,7 +1655,7 @@ extract_bit_field_1 (rtx str_rtx, unsign return NULL; target = extract_fixed_bit_field (int_mode, op0, offset, bitsize, - bitpos, target, unsignedp); + bitpos, target, unsignedp, packedp); return convert_extracted_bit_field (target, mode, tmode, unsignedp); } @@ -1665,6 +1666,7 @@ extract_bit_field_1 (rtx str_rtx, unsign STR_RTX is the structure containing the byte (a REG or MEM). UNSIGNEDP is nonzero if this is an unsigned bit field. + PACKEDP is nonzero if the field has the packed attribute. MODE is the natural mode of the field value once extracted. TMODE is the mode the caller would like the value to have; but the value may be returned with type MODE instead. @@ -1676,10 +1678,10 @@ extract_bit_field_1 (rtx str_rtx, unsign rtx extract_bit_field (rtx str_rtx, unsigned HOST_WIDE_INT bitsize, - unsigned HOST_WIDE_INT bitnum, int unsignedp, rtx target, - enum machine_mode mode, enum machine_mode tmode) + unsigned HOST_WIDE_INT bitnum, int unsignedp, bool packedp, + rtx target, enum machine_mode mode, enum machine_mode tmode) { - return extract_bit_field_1 (str_rtx, bitsize, bitnum, unsignedp, + return extract_bit_field_1 (str_rtx, bitsize, bitnum, unsignedp, packedp, target, mode, tmode, true); } @@ -1695,6 +1697,8 @@ extract_bit_field (rtx str_rtx, unsigned which is significant on bigendian machines.) UNSIGNEDP is nonzero for an unsigned bit field (don't sign-extend value). + PACKEDP is true if the field has the packed attribute. + If TARGET is nonzero, attempts to store the value there and return TARGET, but this is not guaranteed. If TARGET is not used, create a pseudo-reg of mode TMODE for the value. */ @@ -1704,7 +1708,7 @@ extract_fixed_bit_field (enum machine_mo unsigned HOST_WIDE_INT offset, unsigned HOST_WIDE_INT bitsize, unsigned HOST_WIDE_INT bitpos, rtx target, - int unsignedp) + int unsignedp, bool packedp) { unsigned int total_bits = BITS_PER_WORD; enum machine_mode mode; @@ -1769,6 +1773,22 @@ extract_fixed_bit_field (enum machine_mo static bool informed_about_misalignment = false; bool warned; + if (packedp) + { + if (bitsize == total_bits) + warned = warning_at (input_location, OPT_fstrict_volatile_bitfields, + "multiple accesses to volatile structure member" + " because of packed attribute"); + else + warned = warning_at (input_location, OPT_fstrict_volatile_bitfields, + "multiple accesses to volatile structure bitfield" + " because of packed attribute"); + + return extract_split_bit_field (op0, bitsize, + bitpos + offset * BITS_PER_UNIT, + unsignedp); + } + if (bitsize == total_bits) warned = warning_at (input_location, OPT_fstrict_volatile_bitfields, "mis-aligned access used for structure member"); @@ -1971,7 +1991,7 @@ extract_split_bit_field (rtx op0, unsign extract_fixed_bit_field wants offset in bytes. */ part = extract_fixed_bit_field (word_mode, word, offset * unit / BITS_PER_UNIT, - thissize, thispos, 0, 1); + thissize, thispos, 0, 1, false); bitsdone += thissize; /* Shift this part into place for the result. */ Index: stmt.c =================================================================== --- stmt.c (revision 165712) +++ stmt.c (working copy) @@ -1739,7 +1739,7 @@ expand_return (tree retval) xbitpos for the destination store (right justified). */ store_bit_field (dst, bitsize, xbitpos % BITS_PER_WORD, word_mode, extract_bit_field (src, bitsize, - bitpos % BITS_PER_WORD, 1, + bitpos % BITS_PER_WORD, 1, false, NULL_RTX, word_mode, word_mode)); }