From patchwork Fri Oct 18 11:18:48 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Biener X-Patchwork-Id: 284545 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 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 7D98F2C00A2 for ; Fri, 18 Oct 2013 22:19:00 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; q=dns; s=default; b=IAWKAQfYE/ivhXdXpi HshC6i53jK39bzwE8EgtMVl/SPiWUx4vrBSUayfY0mKZO3N6e2paYvnJ75FT0bHg KU2Gl2zJ0l1jltWs1KP6G+mJqBk5RsxmI+KiIusjZn1Gu/FHj5+qKchIKYK4wEko lKV9d6HvvWj7NjbaE5A5U0cOk= 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 :mime-version:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; s=default; bh=ENRejtmW8mlGsS++pO2Pg9Ie 4LY=; b=wS9hNyvhuA1xL5H4fSHfzlEPT9+7uiCvsoXTuV5PrPAna9SO/lBjSsE/ GIhMdN8rYPSLIXuHUtaHhtay2Bs/B2NERBOFl+E3bzGsZhFA/05FHIGPt9pl7kR0 QqXSpwOGzEHp1h8BlKwRu6Wi1wtE29I2/txJQSNrrQskqjhzN3A= Received: (qmail 16984 invoked by alias); 18 Oct 2013 11:18:54 -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 16975 invoked by uid 89); 18 Oct 2013 11:18:54 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-wg0-f53.google.com Received: from mail-wg0-f53.google.com (HELO mail-wg0-f53.google.com) (74.125.82.53) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 18 Oct 2013 11:18:52 +0000 Received: by mail-wg0-f53.google.com with SMTP id y10so3596223wgg.20 for ; Fri, 18 Oct 2013 04:18:48 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.180.91.101 with SMTP id cd5mr324373wib.50.1382095128598; Fri, 18 Oct 2013 04:18:48 -0700 (PDT) Received: by 10.195.12.114 with HTTP; Fri, 18 Oct 2013 04:18:48 -0700 (PDT) In-Reply-To: <002e01cecafd$03809c60$0a81d520$@arm.com> References: <001501ceb906$56da1c00$048e5400$@arm.com> <002701cebb3f$6fcd6ab0$4f684010$@arm.com> <002901cebd9f$5f7cb0a0$1e7611e0$@arm.com> <002e01cecafd$03809c60$0a81d520$@arm.com> Date: Fri, 18 Oct 2013 13:18:48 +0200 Message-ID: Subject: Re: [PATCH]Fix computation of offset in ivopt From: Richard Biener To: "bin.cheng" Cc: "Bin.Cheng" , GCC Patches X-IsSubscribed: yes On Thu, Oct 17, 2013 at 7:52 AM, bin.cheng wrote: > Hi, > As noted in previous messages, GCC forces offset to unsigned in middle end. > It also gets offset value and stores it in HOST_WIDE_INT then uses it in > various computation. In this scenario, It should use int_cst_value to do > additional sign extension against the type of tree node, otherwise we might > lose sign information. Take function fold_plusminus_mult_expr as an > example, the sign information of arg01/arg11 might be lost because it uses > TREE_INT_CST_LOW directly. I think this is the offender of the problem in > this thread. I think we should fix the offender, rather the hacking > strip_offset as in the original patch, so I split original patch into two as > attached, with one fixing the offset of COMPONENT_REF in strip_offset_1, the > other fixing the mentioned sign extension problem. The real issue is that we rely on twos-complement arithmetic to work when operating on pointer offsets (because we convert them all to unsigned sizetype). That makes interpreting the offsets or expressions that compute offsets hard (or impossible in some cases), as you can see from the issues in IVOPTs. The above means that strip_offset_1 cannot reliably look through MULT_EXPRs as that can make an offset X * CST that is "effectively" signed "surely" unsigned in the process. Think of this looking into X * CST as performing a division - whose result is dependent on the sign of X which we lost with our unsigned casting. Now, removing the MULT_EXPR look-through might be too pessimizing ... but it may be worth trying to see if it fixes your issue? In this context I also remember a new bug filed recently of us not folding x /[ex] 4 * 4 to x. In the past I was working multiple times on stopping doing that (make all offsets unsigned), but I never managed to finish ... Richard. > Bootstrap and test on x86/x86_64/arm. Is it OK? > > Thanks. > bin > > Patch a: > 2013-10-17 Bin Cheng > > * tree-ssa-loop-ivopts.c (strip_offset_1): Change parameter type. > Count DECL_FIELD_BIT_OFFSET when computing offset for COMPONENT_REF. > > Patch b: > 2013-10-17 Bin Cheng > > * fold-const.c (fold_plusminus_mult_expr): Use int_cst_value instead > of TREE_INT_CST_LOW. > > gcc/testsuite/ChangeLog > 2013-10-17 Bin Cheng > > * gcc.dg/tree-ssa/ivopts-outside-loop-use-1.c: New test. > >> -----Original Message----- >> From: Richard Biener [mailto:richard.guenther@gmail.com] >> Sent: Wednesday, October 02, 2013 4:32 PM >> To: Bin.Cheng >> Cc: Bin Cheng; GCC Patches >> Subject: Re: [PATCH]Fix computation of offset in ivopt >> >> On Tue, Oct 1, 2013 at 6:13 PM, Bin.Cheng wrote: >> > On Tue, Oct 1, 2013 at 6:50 PM, Richard Biener >> > wrote: >> >> On Mon, Sep 30, 2013 at 7:39 AM, bin.cheng >> wrote: >> >>> >> >>> >> >> >> >> I don't think you need >> >> >> >> + /* Sign extend off if expr is in type which has lower precision >> >> + than HOST_WIDE_INT. */ >> >> + if (TYPE_PRECISION (TREE_TYPE (expr)) <= HOST_BITS_PER_WIDE_INT) >> >> + off = sext_hwi (off, TYPE_PRECISION (TREE_TYPE (expr))); >> >> >> >> at least it would be suspicious if you did ... >> > There is a problem for example of the first message. The iv base if > like: >> > pretmp_184 + ((sizetype) KeyIndex_180 + 1073741823) * 4 I am not sure >> > why but it seems (-4/0xFFFFFFFC) is represented by (1073741823*4). >> > For each operand strip_offset_1 returns exactly the positive number >> > and result of multiplication never get its chance of sign extend. >> > That's why the sign extend is necessary to fix the problem. Or it >> > should be fixed elsewhere by representing iv base with: >> > "pretmp_184 + ((sizetype) KeyIndex_180 + 4294967295) * 4" in the first >> place. >> >> Yeah, that's why I said the whole issue with forcing all offsets to be > unsigned >> is a mess ... >> >> There is really no good answer besides not doing that I fear. >> >> Yes, in the above case we could fold the whole thing differently > (interpret >> the offset of a POINTER_PLUS_EXPR as signed). You can try tracking down >> the offender, but it'll get non-trivial easily as you have to consider the > fact >> that GCC will treat signed operations as having undefined behavior on >> overflow. >> >> So I see why you want to do the extension above (re-interpret the result), > I >> suppose we can live with it but please make sure to add a big fat ??? >> comment before it explaining why it is necessary. >> >> Richard. >> >> >> >> >> The only case that I can think of points to a bug in strip_offset_1 >> >> again, namely if sizetype (the type of all offsets) is smaller than a >> >> HOST_WIDE_INT in which case >> >> >> >> + boffset = int_cst_value (DECL_FIELD_BIT_OFFSET (field)); >> >> + *offset = off0 + int_cst_value (tmp) + boffset / BITS_PER_UNIT; >> >> >> >> is wrong as boffset / BITS_PER_UNIT does not do a signed division >> >> then (for negative boffset which AFAIK does not happen - but it would >> >> be technically allowed). Thus, the predicates like >> >> >> >> + && cst_and_fits_in_hwi (tmp) >> >> >> >> would need to be amended with a check that the MSB is not set. >> > So I can handle it like: >> > >> > + abs_boffset = abs_hwi (boffset); >> > + xxxxx = abs_boffset / BITS_PER_UNIT; >> > + if (boffset < 0) >> > + xxxxx = -xxxxx; >> > + *offset = off0 + int_cst_value (tmp) + xxxxx; >> > >> > Right? >> > >> >> >> >> Btw, the cst_and_fits_in_hwi implementation is odd: >> >> >> >> bool >> >> cst_and_fits_in_hwi (const_tree x) >> >> { >> >> if (TREE_CODE (x) != INTEGER_CST) >> >> return false; >> >> >> >> if (TYPE_PRECISION (TREE_TYPE (x)) > HOST_BITS_PER_WIDE_INT) >> >> return false; >> >> >> >> return (TREE_INT_CST_HIGH (x) == 0 >> >> || TREE_INT_CST_HIGH (x) == -1); } >> >> >> >> the precision check seems totally pointless and I wonder what's the >> >> point of this routine as there is host_integerp () already and >> >> tree_low_cst instead of int_cst_value - oh, I see, the latter >> >> forcefully sign-extends .... that should make the extension not >> >> necessary. >> > See above. >> > >> > Thanks. >> > bin Index: gcc/fold-const.c =================================================================== --- gcc/fold-const.c (revision 203267) +++ gcc/fold-const.c (working copy) @@ -7270,8 +7270,8 @@ fold_plusminus_mult_expr (location_t loc, enum tre HOST_WIDE_INT int01, int11, tmp; bool swap = false; tree maybe_same; - int01 = TREE_INT_CST_LOW (arg01); - int11 = TREE_INT_CST_LOW (arg11); + int01 = int_cst_value (arg01); + int11 = int_cst_value (arg11); this is not correct - it will mishandle all large unsigned numbers.