From patchwork Mon Feb 15 09:05:28 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Biener X-Patchwork-Id: 582783 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]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 10629140322 for ; Mon, 15 Feb 2016 20:05:43 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=o3e5nAoD; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:in-reply-to:message-id:references :mime-version:content-type; q=dns; s=default; b=cYayOWNQTzaeIggp 7Tbny+TvYE07ZiedJvkWsm2riHoY+1w88MMn9Y3/+v2GjwNG7sqKrYLv8gZsxbJl vMBf4yZskK7ETpqVJV8Kp0S/mSy4jq4TgTIgKkJrmKqPF0Foo4plul1KQABBAoM8 NkxJvs4MJhglbwfIaxpGMU4p82s= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:in-reply-to:message-id:references :mime-version:content-type; s=default; bh=WM13Oa7as2Z9svkTu+CrMf I8yCo=; b=o3e5nAoDi0f+YfbG5iLCYM/N/SYJSAqAE+ia/RjtbeusLTfz3FCd1D HhVguE5btbcDlkUKHwp8t35554HwZuS3rnRSeXfz5xdZg0CDpaaEcefPgZHDMihF CAZZfvY5hM/pqp0/h5WhlYIY/8PloGW0awmMrJXzheyB9wr+J5190= Received: (qmail 74345 invoked by alias); 15 Feb 2016 09:05:34 -0000 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 Received: (qmail 74336 invoked by uid 89); 15 Feb 2016 09:05:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.6 required=5.0 tests=BAYES_50, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_NONE, RP_MATCHES_RCVD, SPF_PASS autolearn=no version=3.3.2 spammy=nail, pac, sk:TYPE_MA, sk:type_ma X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (CAMELLIA256-SHA encrypted) ESMTPS; Mon, 15 Feb 2016 09:05:32 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 9DA76AB5D; Mon, 15 Feb 2016 09:05:28 +0000 (UTC) Date: Mon, 15 Feb 2016 10:05:28 +0100 (CET) From: Richard Biener To: Eric Botcazou cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Avoid bugs like PR68273 to trigger In-Reply-To: <5056495.Dz861ZsnJV@polaris> Message-ID: References: <3621650.BTtq9jcdsO@polaris> <5056495.Dz861ZsnJV@polaris> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 On Sun, 14 Feb 2016, Eric Botcazou wrote: > > No, but if there is none left why would you want to "fix" SRA? > > As expected, it seems that the make_ssa_name_fn kludge is not sufficient, so > I'm proposing to disable the PR65310 one-liner for selected targets, using the > function_arg_boundary hook, until after we have a clear way out of this mess. > > Here's a summary of the situation: > > targhooks.c:default_function_arg_boundary N > aarch64/aarch64.c:aarch64_function_arg_boundary Y > arm/arm.c:arm_function_arg_boundary N > c6x/c6x.c:c6x_function_arg_boundary Y > epiphany/epiphany.c:epiphany_function_arg_boundary Y > frv/frv.c:frv_function_arg_boundary N > i386/i386.c:ix86_function_arg_boundary N > ia64/ia64.c:ia64_function_arg_boundary Y > iq2000/iq2000.c:iq2000_function_arg_boundary Y > m32c/m32c.c:m32c_function_arg_boundary N > mcore/mcore.c:mcore_function_arg_boundary N > mips/mips.c:mips_function_arg_boundary Y > msp430/msp430.c:msp430_function_arg_boundary N > nds32/nds32.c:nds32_function_arg_boundary Y > pa/pa.c:pa_function_arg_boundary N > rl78/rl78.c:rl78_function_arg_boundary N > rs6000/rs6000.c:rs6000_function_arg_boundary Y (aggr, AIX/ELFv2) > rx/rx.c:rx_function_arg_boundary Y > sparc/sparc.c:sparc_function_arg_boundary Y (64-bit) > tilegx/tilegx.c:tilegx_function_arg_boundary Y > tilepro/tilepro.c:tilepro_function_arg_boundary Y > xtensa/xtensa.c:xtensa_function_arg_boundary Y > > A 'Y' means that the return value of function_arg_boundary depends on the > alignment of the type it is directly passed (e.g. not on its main variant). > 'aggr' means for aggregate types only, the other modifiers are ABIs. > > So if we add a test based on function_arg_boundary, we'll effectively disable > the PR65310 one-liner in some cases for the following targets: > aarch64, c6x, epiphany, ia64, iq2000, mips, nds32, rs6000 (aggr), rx, sparc, > tilegx, tilepro, xtensa > > If we additionally test STRICT_ALIGNMENT, the set of targets shrinks to: > c6x, epiphany, ia64, iq2000, mips, nds32, sparc, tilegx, tilepro, xtensa > > MIPS being in both sets, this will fix PR68273 in both cases. I agree with Jakub on this (obviously), still a comment on the patch: > Index: tree-sra.c > =================================================================== > --- tree-sra.c (revision 233338) > +++ tree-sra.c (working copy) > @@ -1681,9 +1681,22 @@ build_ref_for_offset (location_t loc, tr > misalign = (misalign + offset) & (align - 1); > if (misalign != 0) > align = (misalign & -misalign); > - if (align != TYPE_ALIGN (exp_type)) > + > + /* Misaligning a type is generally OK (if it's naturally aligned). */ > + if (align < TYPE_ALIGN (exp_type)) > exp_type = build_aligned_type (exp_type, align); So you simply assume that exp_type is naturally aligned here. I think you should test align < TYPE_ALIGN (TYPE_MAIN_VARIANT (exp_type)) here, no? > + /* Overaligning it can be problematic because of calling conventions. */ > + else if (align > TYPE_ALIGN (exp_type)) > + { > + tree aligned_type = build_aligned_type (exp_type, align); > + if (targetm.calls.function_arg_boundary (TYPE_MODE (aligned_type), > + aligned_type) > + == targetm.calls.function_arg_boundary (TYPE_MODE (exp_type), > + exp_type)) And if you get enough supporters to apply this kind of workaround I'd prefer it to be in build_aligned_type itself, basically refusing to build over-aligned types. And I'd rather make this controlled by an internal flag that backends should consciously set (aka a target hook). The above is simply a bit too ugly IMHO and looks incomplete(?), cannot even the cummulative args machinery end up with type-align specifics or are you sure those can only be triggered from function_arg_boundary? Note the real issue is overaligned register types. I've tried so you should be able to point to a specific pass doing the wreckage and provide preprocessed source and a cross-compiler setup to investigate. > + exp_type = aligned_type; > + } > + > mem_ref = fold_build2_loc (loc, MEM_REF, exp_type, base, off); > REF_REVERSE_STORAGE_ORDER (mem_ref) = reverse; > if (TREE_THIS_VOLATILE (prev_base)) > Index: tree-ssanames.c > =================================================================== > --- tree-ssanames.c (revision 233338) > +++ tree-ssanames.c (working copy) > @@ -286,7 +286,7 @@ make_ssa_name_fn (struct function *fn, t > > if (TYPE_P (var)) > { > - TREE_TYPE (t) = TYPE_MAIN_VARIANT (var); > + TREE_TYPE (t) = var; > SET_SSA_NAME_VAR_OR_IDENTIFIER (t, NULL_TREE); > } > else Note that this change was desirable on its own as this is how I expect types to end up in the middle-end after we fix LTO debug and thus can free more of the "language specific" type pieces in the middle-end. Richard. Index: tree-ssa.c =================================================================== --- tree-ssa.c (revision 233369) +++ tree-ssa.c (working copy) @@ -936,6 +936,22 @@ verify_ssa (bool check_modified_stmt, bo name, stmt, virtual_operand_p (name))) goto err; } + + if (! SSA_NAME_VAR (name) + && TYPE_MAIN_VARIANT (TREE_TYPE (name)) != TREE_TYPE (name)) + { + error ("type is not main variant"); + goto err; + } + but that's not too happy (to verify the change below). But verifying we don't have over-aligned (or under-aligned) registers would be a good thing and I think all registers should have main-variant types. So I'd rather nail down the case that still gets through with the below patch using sth like the above (well, as said, adjust it to check all SSA names and TYPE_ALING == TYPE_ALIGN (TYPE_MAIN_VARIANT ())). At least it seems you have a testcase that reproduces the error