From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30744 invoked by alias); 18 Mar 2012 10:16:29 -0000 Received: (qmail 30735 invoked by uid 22791); 18 Mar 2012 10:16:28 -0000 X-SWARE-Spam-Status: No, hits=-2.4 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW 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; Sun, 18 Mar 2012 10:16:13 +0000 Received: by wibhq7 with SMTP id hq7so2060773wib.8 for ; Sun, 18 Mar 2012 03:16:12 -0700 (PDT) Received: by 10.180.104.230 with SMTP id gh6mr11434125wib.22.1332065771761; Sun, 18 Mar 2012 03:16:11 -0700 (PDT) Received: from localhost (rsandifo.gotadsl.co.uk. [82.133.89.107]) by mx.google.com with ESMTPS id ff2sm24941769wib.9.2012.03.18.03.16.10 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 18 Mar 2012 03:16:11 -0700 (PDT) From: Richard Sandiford To: Mike Stump Mail-Followup-To: Mike Stump ,gcc-patches Patches , rdsandiford@googlemail.com Cc: gcc-patches Patches Subject: Re: remove wrong code in immed_double_const In-Reply-To: (Mike Stump's message of "Sat, 17 Mar 2012 17:29:03 -0700") References: <5FF5A724-3FE1-4E97-8124-542A0B8259FE@comcast.net> <87obrvd6fh.fsf@talisman.home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) Date: Sun, 18 Mar 2012 10:16:00 -0000 Message-ID: <87haxmgqoo.fsf@talisman.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2012-03/txt/msg01212.txt.bz2 Mike Stump writes: >> We currently only support constant integer >> widths <= 2*HOST_BITS_PER_WIDE_INT, and the assert is correctly >> triggering if we try to build a wider constant. > > Can you give me a concrete example of what will fail with the constant > 0 generated by: > > return GEN_INT (i0); > > when the mode is 2*HOST_BITS_PER_WIDE_INT? When the mode is larger than this? I think the assert is more saying that things have already gone wrong if we have reached this point. immed_double_const is only designed to handle two HOST_WIDE_INTs, so what exactly is the caller trying to do? E.g. do we apply sign or zero extension to the 2 HOST_WIDE_INTs? If we're going to remove the assert, we need to define stuff like that. All ones is a pretty common constant, so if I was going to guess, I'd have expected we'd sign extend, just like we do for CONST_INT. So immed_double_const (-1, -1, int_mode) would always be GEN_INT (-1). But if we do that, we need to define the high HOST_WIDE_INT of a CONST_DOUBLE to be sign-extended too. And at the moment we don't; the CONST_DOUBLE case of simplify_immed_subreg says: /* It shouldn't matter what's done here, so fill it with zero. */ for (; i < elem_bitsize; i += value_bit) *vp++ = 0; Compare with the CONST_INT case: /* CONST_INTs are always logically sign-extended. */ for (; i < elem_bitsize; i += value_bit) *vp++ = INTVAL (el) < 0 ? -1 : 0; So... > See, we already shorten the width of wide constants and expect that to > work. This is just another instance of shortening. Now, just to see > what lurks in there, I when through 100% of the uses of CONST_DOUBLE > in *.c to get a feel for what might go wrong, the code is as benign as > I expected, every instance I saw, looked reasonably safe. It doesn't > get any better than this. ...I'm not sure about. >From a quick grep, it looks like the case where I saw the immed_double_const assert triggering -- the expansion of INTEGER_CST -- is the only case where it could trigger. So I suppose the question shifts to the tree level. Are callers to build_int_cst_wide and double_int_to_tree already expected to check that constants wider than a double_int would not be truncated? If so, then great. And if so, what are the rules there regarding sign vs. zero extension beyond the high HOST_WIDE_INT? >> FWIW, here's another case where this came up: >> >> http://gcc.gnu.org/ml/gcc-patches/2011-03/msg01220.html > > Yes, and surprisingly, or not it is the exact same case as I can into. I'd expect it to be a slightly different case. The original testcase was a union constructor that was unnecessarily initialised to zero. That has since been fixed. Richard