From: Steven Bosscher <stevenb.gcc@gmail.com>
To: Eric Botcazou <ebotcazou@adacore.com>
Cc: gcc-patches@gcc.gnu.org, Paolo Bonzini <bonzini@gnu.org>,
Dominique Dhumieres <dominiq@lps.ens.fr>,
hubicka@ucw.cz, Richard Henderson <rth@redhat.com>,
Jeff Law <law@redhat.com>
Subject: Re: Fix twolf -funroll-loops -O3 miscompilation (a semi-latent web.c bug)
Date: Mon, 03 Dec 2012 20:20:00 -0000 [thread overview]
Message-ID: <CABu31nPTc7SKZBaUeEy=FsxrKMWzRNtg+D07hatOgrZK_+o-AQ@mail.gmail.com> (raw)
In-Reply-To: <8323491.RGjWGtIWxN@polaris>
On Mon, Dec 3, 2012 at 7:23 PM, Eric Botcazou wrote:
>> >> 2. gcse.c: gcse_emit_move_after added notes, but none of them was very
>> >> useful as far as I could tell, and almost all of them turned
>> >> self-referencing after CPROP. So I propose we just never add notes in
>> >> this case.
>> >
>> > gcse_emit_move_after also preserves existing notes. Are they
>> > problematic?
>> Yes, they tend to be invalid after PRE because the registers used in
>> the PRE'd expression usually are not live anymore (making the note
>> invalid). Sometimes CPROP "re-validates" the notes, but it doesn't
>> seem wise to me to rely on that.
>
> So the compiler doesn't bootstrap with the gcse.c patch you posted earlier in
> the thread? Still this seems too bold to me, the note datum could be a
> constant and should be preserved in this case.
You mean the patch at
http://gcc.gnu.org/ml/gcc-patches/2012-11/msg02275.html right?
I haven't tried that other patch. I'll test that one.
>> >> 3. cprop.c: It seems to me that the purpose here is to propagate
>> >> constants. If a reg could not be propagated, then the REG_EQUAL note
>> >> will not help much either. Propagating constants via REG_EQUAL notes
>> >> still helps folding comparisons sometimes, so I'm proposing we only
>> >> propagate those. As a bonus: less garbage RTL because a
>> >> cprop_constant_p can be shared.
>> >
>> > That seems a bit radical to me, especially in try_replace_reg which is
>> > used for copy propagation as well. In cprop_jump, why is attaching a
>> > note to the jump problematic?
>>
>> Most of the time a note from copy-propagation was not valid because
>> the copy-prop'd reg was not live at the point of the note.
>
> This one I think we should drop for now, or just avoid the self-referential
> case. There is a comment explicitly saying that the REG_EQUAL note added by
> try_replace_reg are part of the algorithm.
I suppose so. But this was all added before RTL fwprop and way before
GIMPLE optimizations. Avoiding the self-referential case is just more
difficult to do, quite expensive (have to scan the SET_SRC pattern),
and AFAICT doesn't bring much pay-off.
I'll prepare something to avoid the self-referential case, but I think
we're making our lives complicated for no good reason.
>> Not really. It uses single_set in a few places, including
>> delete_trivially_dead_insns and cse_extended_basic_block.
>>
>> > so it seems like we're back to the earlier
>> > trick of using df_note_add_problem to clean up pre-existing REG_EQ*
>> > notes.
>> Again: Not really. I also bootstrapped&tested without the cse.c change.
>
> The cse.c hunk is OK then.
Thanks, I'll commit it separately.
>> I plan (and promise ;-) to add a REG_EQ* note verifier for GCC 4.9.
>
> Thanks (no need to promise though :-), that will be helpful. In the meantime,
> I don't think that we should aim for perfection in 4.8, these REG_EQ* notes
> and their quirks have been with us for a long time...
Well, yes they've been with us for a long time, but my LR_RD change
exposed all these problems that were simply hidden before. I think
we're safe for GCC 4.8 but I don't feel comfortable about this
situation...
Ciao!
Steven
next prev parent reply other threads:[~2012-12-03 20:20 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-11-18 23:15 Dominique Dhumieres
2012-11-19 9:50 ` Steven Bosscher
2012-11-19 11:27 ` Eric Botcazou
2012-11-19 20:35 ` Steven Bosscher
2012-11-19 21:20 ` Steven Bosscher
2012-11-19 21:52 ` Eric Botcazou
2012-11-19 22:03 ` Steven Bosscher
2012-11-19 22:44 ` Eric Botcazou
2012-11-19 22:48 ` Steven Bosscher
2012-11-23 22:46 ` Steven Bosscher
2012-11-24 1:10 ` Steven Bosscher
2012-11-25 20:44 ` Steven Bosscher
2012-11-25 22:38 ` Steven Bosscher
2012-11-26 14:38 ` Dominique Dhumieres
2012-11-26 15:46 ` Steven Bosscher
2012-11-27 9:58 ` Eric Botcazou
2012-11-27 10:35 ` Steven Bosscher
2012-11-27 12:01 ` Steven Bosscher
2012-11-27 12:29 ` Steven Bosscher
2012-11-27 13:04 ` Steven Bosscher
2012-11-27 14:25 ` Dominique Dhumieres
2012-11-27 14:49 ` Steven Bosscher
2012-11-27 13:48 ` Dominique Dhumieres
2012-11-27 14:33 ` Paolo Bonzini
2012-11-27 16:59 ` Eric Botcazou
2012-11-27 23:29 ` Steven Bosscher
2012-11-27 23:50 ` Eric Botcazou
2012-11-27 23:54 ` Steven Bosscher
2012-11-27 23:59 ` Steven Bosscher
2012-11-28 0:43 ` Steven Bosscher
2012-11-28 7:46 ` Eric Botcazou
2012-11-28 15:57 ` Steven Bosscher
2012-11-28 22:12 ` Eric Botcazou
2012-11-28 23:54 ` Steven Bosscher
2012-12-01 14:57 ` Eric Botcazou
2012-12-01 16:45 ` Steven Bosscher
2012-12-03 18:26 ` Eric Botcazou
2012-12-03 20:20 ` Steven Bosscher [this message]
2012-12-03 21:12 ` Eric Botcazou
2012-12-03 23:28 ` Steven Bosscher
2012-12-03 20:15 ` Paolo Bonzini
2012-11-19 21:29 ` Eric Botcazou
-- strict thread matches above, loose matches on Subject: below --
2012-10-12 20:14 Jan Hubicka
2012-10-12 20:36 ` Markus Trippelsdorf
2012-10-12 20:44 ` Steven Bosscher
2012-10-12 21:16 ` Jan Hubicka
2012-10-12 21:19 ` Steven Bosscher
2012-10-12 21:31 ` Jan Hubicka
2012-10-12 22:41 ` Steven Bosscher
2012-10-14 9:13 ` Paolo Bonzini
2012-10-14 21:27 ` Steven Bosscher
2012-10-14 21:35 ` Eric Botcazou
2012-10-14 21:45 ` Steven Bosscher
2012-10-15 8:14 ` Paolo Bonzini
2012-10-15 8:23 ` Steven Bosscher
2012-10-15 8:35 ` Paolo Bonzini
2012-10-15 8:38 ` Steven Bosscher
2012-10-15 10:49 ` Steven Bosscher
2012-10-15 12:28 ` Paolo Bonzini
2012-10-15 13:19 ` Steven Bosscher
2012-10-15 13:29 ` Paolo Bonzini
2012-10-15 13:49 ` Steven Bosscher
2012-10-16 10:35 ` Steven Bosscher
2012-10-16 11:05 ` Steven Bosscher
2012-10-16 11:42 ` Paolo Bonzini
2012-10-16 22:57 ` Steven Bosscher
2012-10-19 5:14 ` Bin.Cheng
2012-10-12 21:05 ` Steven Bosscher
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CABu31nPTc7SKZBaUeEy=FsxrKMWzRNtg+D07hatOgrZK_+o-AQ@mail.gmail.com' \
--to=stevenb.gcc@gmail.com \
--cc=bonzini@gnu.org \
--cc=dominiq@lps.ens.fr \
--cc=ebotcazou@adacore.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=hubicka@ucw.cz \
--cc=law@redhat.com \
--cc=rth@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).