From patchwork Thu Jan 29 01:58:12 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Hale Wang X-Patchwork-Id: 434278 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 7C77D14021A for ; Thu, 29 Jan 2015 12:59:10 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:references:in-reply-to:subject:date:message-id :mime-version:content-type; q=dns; s=default; b=dyBhdrEmmNe3ulIu /dKdfAIOYDqjUKeIuH1//76O+/DH55vbOy6vHVdgWW6ic6HRvfHSzuaQ+2yEzVjp 006KEzaPMVc4RZuWI792/YVYcdBFveXJrXSOuii9P94JNa1Dlb3gx19G7JTzEqBq cNAeRI2m0K3DVexYWD5kZpAB9Cc= 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:from :to:cc:references:in-reply-to:subject:date:message-id :mime-version:content-type; s=default; bh=q7+Yc6Rv1zekE+J/891QTm E9ras=; b=IEqRlO6Py9SjLys7EoBzRAwqBtvR+z+G1ghs1AyzH7Sa7bh/Ux6IX9 S77YVqyA1DyjK89aGFt9h5EfITuoUA5K9JZ71xTGXL8U+AmZ0NUjhWKGWlwDmraj OhRdlC9P7ThUKIBgsptlxYrFzX9T1ShMv+MRUOpD+Q5ItQyhKHINc= Received: (qmail 21651 invoked by alias); 29 Jan 2015 01:58:40 -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 21595 invoked by uid 89); 29 Jan 2015 01:58:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.1 required=5.0 tests=AWL, BAYES_00, SPF_PASS autolearn=ham version=3.3.2 X-HELO: service87.mimecast.com Received: from service87.mimecast.com (HELO service87.mimecast.com) (91.220.42.44) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 29 Jan 2015 01:58:27 +0000 Received: from cam-owa1.Emea.Arm.com (fw-tnat.cambridge.arm.com [217.140.96.140]) by service87.mimecast.com; Thu, 29 Jan 2015 01:58:24 +0000 Received: from shawin221 ([10.1.255.212]) by cam-owa1.Emea.Arm.com with Microsoft SMTPSVC(6.0.3790.3959); Thu, 29 Jan 2015 01:58:21 +0000 From: "Hale Wang" To: "Hale Wang" , "'Segher Boessenkool'" Cc: "GCC Patches" References: <001e01d0394e$470bc390$d5234ab0$@arm.com> <20150126190641.GA32345@gate.crashing.org> <004a01d039e4$524dedf0$f6e9c9d0$@arm.com> <20150127045142.GA9815@gate.crashing.org> <004b01d039f4$41df96c0$c59ec440$@arm.com> In-Reply-To: <004b01d039f4$41df96c0$c59ec440$@arm.com> Subject: RE: [PATCH] [gcc, combine] PR46164: Don't combine the insns if a volatile register is contained. Date: Thu, 29 Jan 2015 09:58:12 +0800 Message-ID: <007001d03b67$0bd51a90$237f4fb0$@arm.com> MIME-Version: 1.0 X-MC-Unique: 115012901582400501 X-IsSubscribed: yes Hi Segher, I have updated the patch as you suggested. Both the patch and the changelog are attached. By the way, the test case provided by Tim Pambor in PR46164 was a different bug with PR46164. So I resubmitted the bug in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64818. And this patch is just used to fix this bug. Is it OK for you? Thanks, Hale gcc/ChangeLog: 2015-01-27 Segher Boessenkool Hale Wang PR rtl-optimization/64818 * combine.c (can_combine_p): Don't combine the insn if the dest of insn is a user specified register. gcc/testsuit/ChangeLog: 2015-01-27 Segher Boessenkool Hale Wang PR rtl-optimization/64818 * gcc.target/arm/pr64818.c: New test. +/* { dg-final { scan-assembler "\[\\t \]+mov\ r1,\ r1" } } */ > > On Tue, Jan 27, 2015 at 11:49:55AM +0800, Hale Wang wrote: > > > > Hi Hale, > > > You are correct. Just "-O1" reproduces this problem. > > > However it's a combine bug which is related to the combing user > > > specified register into inline-asm. > > > > Yes, it is. But the registers the testcase uses exist on any ARM > > version > there > > is as far as I know, so not specifying specific model and ABI should > > give > wider > > test coverage (if anyone actually builds and/or tests more than the > default, > > of course :-) ) > > > > > > Could you try this patch please? > > > > > > Your patch rejected the combine 98+43, that's correct. > > > > Excellent, thanks for testing. > > > > > However, Jakub > > > pointed out that preventing that to be combined would be a serious > > > regression on code quality. > > > > I know; I needed to think of some good way to detect register > > variables > (they > > aren't marked specially in RTL). I think I found one, for combine > > that > is; if we > > need to detect it in other passes too, we probably need to put another > flag > > on it, or something. > > > > > Andrew Pinski suggested: can_combine_p would reject combing into an > > > inline-asm to prevent this issue. And I have updated the patch. What > > > do you think about this change? > > > > That will regress combining anything else into an asm. It will > > disallow combining asms _at all_, if we really wanted that we should > > simply not > build > > LOG_LINKS for them. But it hurts optimisation (for simple "r" > > constraints > it is > > not a real problem, RA should take care of it, but for anything else > > it > is). > > > > Updated patch below. A user variable that is also a hard register can > only > > happen in a few cases: 1) a register variable, the case we are after; > > 2) > an > > argument for the current function that was propagated into a user > > variable (something combine should not do at all, it hinders good > > register > allocation, > > but it does anyway on most targets). > > > > Do you want to take this or shall I? This is not a regression, so it > probably > > should wait for stage1 :-( > > > > Your solution is very good. I will test this patch locally and send out the result > ASAP. > Thanks, > > Hale > > > > > Segher > > diff --git a/gcc/combine.c b/gcc/combine.c index 5c763b4..6901ac2 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -1904,6 +1904,12 @@ can_combine_p (rtx_insn *insn, rtx_insn *i3, rtx_insn *pred ATTRIBUTE_UNUSED, set = expand_field_assignment (set); src = SET_SRC (set), dest = SET_DEST (set); + /* Don't combine if dest contains a user specified register, because the + user specified register (same with dest) in i3 would be replaced by the + src of insn which might be different with the user's expectation. */ + if (REG_P (dest) && REG_USERVAR_P (dest) && HARD_REGISTER_P (dest)) + return 0; + /* Don't eliminate a store in the stack pointer. */ if (dest == stack_pointer_rtx /* Don't combine with an insn that sets a register to itself if it has diff --git a/gcc/testsuite/gcc.target/arm/pr64818.c b/gcc/testsuite/gcc.target/arm/pr64818.c new file mode 100644 index 0000000..bddd846 --- /dev/null +++ b/gcc/testsuite/gcc.target/arm/pr64818.c @@ -0,0 +1,30 @@ +/* { dg-do compile } */ +/* { dg-options "-O1" } */ + +char temp[16]; +extern int foo1 (void); + +void foo (void) +{ + int i; + int len; + + while (1) + { + len = foo1 (); + register int a asm ("r0") = 5; + register char *b asm ("r1") = temp; + register int c asm ("r2") = len; + asm volatile ("mov %[r0], %[r0]\n mov %[r1], %[r1]\n mov %[r2], %[r2]\n" + : "+m"(*b) + : [r0]"r"(a), [r1]"r"(b), [r2]"r"(c)); + + for (i = 0; i < len; i++) + { + if (temp[i] == 10) + return; + } + } +} +