From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 32339 invoked by alias); 27 Nov 2012 10:35:25 -0000 Received: (qmail 32222 invoked by uid 22791); 27 Nov 2012 10:35:25 -0000 X-SWARE-Spam-Status: No, hits=-5.2 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-la0-f47.google.com (HELO mail-la0-f47.google.com) (209.85.215.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 27 Nov 2012 10:35:20 +0000 Received: by mail-la0-f47.google.com with SMTP id u2so9444048lag.20 for ; Tue, 27 Nov 2012 02:35:19 -0800 (PST) Received: by 10.152.106.110 with SMTP id gt14mr14019178lab.1.1354012518981; Tue, 27 Nov 2012 02:35:18 -0800 (PST) MIME-Version: 1.0 Received: by 10.112.88.99 with HTTP; Tue, 27 Nov 2012 02:34:38 -0800 (PST) In-Reply-To: <38690302.TaVonSBHVs@polaris> References: <20121118231540.726263BABA@mailhost.lps.ens.fr> <38690302.TaVonSBHVs@polaris> From: Steven Bosscher Date: Tue, 27 Nov 2012 10:35:00 -0000 Message-ID: Subject: Re: Fix twolf -funroll-loops -O3 miscompilation (a semi-latent web.c bug) To: Eric Botcazou Cc: gcc-patches@gcc.gnu.org, Dominique Dhumieres , bonzini@gnu.org, hubicka@ucw.cz, Richard Henderson , Jeff Law Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes 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-11/txt/msg02183.txt.bz2 On Tue, Nov 27, 2012 at 10:55 AM, Eric Botcazou wrote: >> Or rather this one. Same hammer, different color. It turns out that >> the rtlanal.c change caused problems, so I've got to use a home-brewn >> equivalent of remove_reg_equal_equiv_notes_for_regno... >> >> Test case is unchanged so I'm omitting it here. >> >> Ciao! >> Steven >> >> >> gcc/ >> * loop-unroll.c (struct iv_to_split): Add new 'orig_var' member. >> (analyze_iv_to_split_insn): Record it. >> (maybe_strip_eq_note_for_split_iv): New function to remove REG_EQUAL >> notes that refer to IVs that are being split. >> (apply_opt_in_copies): Use maybe_strip_eq_note_for_split_iv. Twice. >> Use FOR_BB_INSNS_SAFE. > > That's fine with me, thanks. You might want to defer applying it until the > reason why it isn't apparently sufficient for aermod.f90 is understood though. Thanks. Yes, I'm first going to try and figure out why this doesn't fix aermod for Dominique. I suspect the problem is in variable expansion in the unroller. A previous patch of mine updates REG_EQUAL notes in variable expansion, but it doesn't clean up notes that refer to maybe dead registers. I have to say, I've got a uncomfortable feeling about REG_EQUAL notes not being allowed to refer to dead registers. In the case at hand, the code basically does: S1: r1 = r2 + 0x4 // r2 is the reg from split_iv, r1 was the original IV S2: r3 = mem[r1] S3: if ... goto L1; S4: r4 = r3 // REG_EQUAL mem[r1] L1: S5: r1 = r2 + 0x8 At S4, r1 is not live in the usual meaning of register liveness, but the DEF from S1 still reaches the REG_EQUAL note. This situation is not only created by loop unrolling. At least gcse.c (PRE), loop-invariant.c, cse.c, dce.c and combine.c can create situations like the above. ifcvt.c jumps through hoops to avoid it (see e.g. PR46315, which you fixed). Most of the problems are papered over by occasional calls to df_note_add_problem from some passes, so that df_remove_dead_eq_notes cleans up any notes referencing dead registers. But if we really want to declare this kind of REG_EQUAL note reference to a dead register invalid RTL (which is something at least you, Paolo, and me agree on), and we expect passes to clean up their own mess by either updating or removing any notes they invalidate, then df_remove_dead_eq_notes shouldn't be necessary for correctness because all notes it encounters should be valid (being updated by passes). Removing df_remove_dead_eq_notes breaks bootstrap on the four targets I tried it on (x86_64, powerpc64, ia64, and hppa). That is, breaks *without* -funroll-loops, and *without* -fweb. With a hack to disable pass_initialize_regs, bootstrap still fails, but other files are miscompiled. With another hack on top to disable CSE2, bootstrap still fails, and yet other files are miscompiled. What I'm really trying to say, is that even when I fix this particular issue with aermod (which is apparently 3 issues: the one I fixed last month for x86_64, the one fixed with the patch in this thread for SPARC, and the yet-to-be-identified problem Dominique is still seeing on darwin10) then it's likely other, similar bugs will show up. Bugs that are hard to reproduce, dependent on the target's RTL, and difficult to debug as they are usually wrong-code bugs on larger test cases. We really need a more robust solution. I've added Jeff and rth to the CC, looking for opinions/thoughts/suggestions/$0.02s. Ciao! Steven