Wednesday 19 April 2006 08:24, Mark Mitchell wrote:
Steven Bosscher wrote:
Monday 10 April 2006 14:19, Paolo Bonzini wrote:
>We're deep in stage 3, and I really would like to finish the
>>
>fwprop merge, stage 2 project:
>
>
>
This is a stage2 project, but very few people have commented on
this set of patches so far. I think that is unfortunate, because
these patches bring rather considerable compile time improvements
and opportunities for further clean-ups.
I don't think I can competently review these patches, but I would like
to see them reviewed. Even though we're in Stage 3, we should try to
clear the decks for things that have been already submitted.
Ping ping ping ping ping ping!!!!
Gr.
Steven

5/7/06, Roger Sayle <roger (AT) eyesopen (DOT) comwrote:

Hi Steven (and Paolo),

Sun, 7 May 2006, Steven Bosscher wrote:
>fwprop merge, stage 2 project:
>
>
Ping ping ping ping ping ping!!!!

Constructively, it would help significantly if someone updated these
patches to mainline, rebootstrapped and regression tested them and
resubmitted them.

I'm doing that as we speak. I have bootstrapped (multilib) the
patches on x86_64 for c,c++,objc,fortran,ada,java and I'm waiting for
the regression tests to complete. Then I'll post the updated patch
with the test results (which should IMVH be a requirement FWIW).

There have been a significant number of changes
in the last few months, and it seems only reasonable to ensure that
significant changes in stage3 apply cleanly and don't cause regressions.

It would have been reasonable to don't let a patch go without any
reply whatsoever for four months, too.

The updates are a mere 3 changes that affect these two patches:
- CLEANUP_PRE_LP is gone
- execute in the tree_pass structure now is unsigned int
- validate_change_maybe_volatile is gone so that one hunk doesn't apply

I'm not sure why Paolo isn't pinging these patches himself,

Paolo _has_ pinged his patches. Many times. And besides, I'm also an
author of this new pass, so they are my patches too.

>This is one reason why PINGs
by interested third parties (other than the original contributor)
carry slightly less weight.

RTFfwprop.c file and see my name on it so I don't consider myself a
third party, thank you very much.

These submissions claim to be mostly performance neutral on SPEC, with a
slight hit for PPC,

No slight hit on PPC. I appreciate you reply to my ping, but it would
have been helpful if you've read Paolo's complete story. The PPC
regression was fixed already even before these fwprop patches were
posted.

Personally, I'd love to see the 3% compile-time improvements from
disabling path-following in CSE provided that the patch is safe and
the performance impact on most platforms is negligible. However,
there are already complaints about the reasoning used to accept/reject
significant changes in stage3, and whether they are truly ready for
GCC just becuase they are pinged continually.

First of all, this work was finished in september last year.

Second, it has been on a branch (the dataflow branch) for months now,
which has been tested on a large number of targets.

Third, this is not a stage3 contribution, and I can't help that the
reviewers don't do their job.

If I post a patch in stage2, and early stage2 too if I may add, then
you should not slap me with "significant changes in stage3", but
instead ask yourself "why didn't I take the responsibility that I have
as a reviewer to comment on the patch?". IMH you've forfeited your
right to complain about timing if a patch has been pending as long as
these two.

The postings for these patches also reported that these changes had
only been bootstrapped and regression tested on i686-pc-linux-gnu.
If someone was serious about their patches, they'd perhaps do slightly
more than the required minimum testing.

But they had previously been bootstrapped and tested by me on
{i686,x86_64,ppc,ppc64,ia64}-suse-linux-gnu. Plus SPEC on the two
x86* targets. Plus nullstone on x86*. Plus PPC testing by David
Edelsohn. Plus on a number of targets on the dataflow-branch. That
_is_ slightly more than the required minimum testing.

I realize you're not a mind reader, but when this patch was posted,
just testing on one target was enough for a patch contribution. Now
you are raising the bar because, essentially, you and your fellow
reviewers have failed. That is the way I see it at least.

There's also the issue of breakage and fallout. Paolo's most recent
patch is still causing mainline regressions on x86_64, see the post
and notice
the "PING^n fwprop" in the title.

If you had read beyond the subject, you'd have seen that the
regressions mentioned concern a reload change, which also had to be
pinged because nobody is reviewing patches. That message you point to
had nothing to do with fwprop other than the subject line.

btw, I seem to recall that all your patches in the tree overflow
mess also have caused numerous regressions. The difference between
yours and fwprops (if any, and I doubt it) is that you can approve
your own patches so that they are commited when they are ready, and
other people like Paolo and me have to wait until some reviewer is so
kind to take a little time to look at some patch. Which obviously
doesn't happen enough as proven by the huge number of pending patches
in e.g. the patch tracker. So you have time to fix whatever you've
broken, and we don't.

In fact, I'm fed up with your I-know-better attitude here. Many of
the points you bring up have little or no relation to fwprop as far as
I can see, and none of them would have been particularly relevant if
these patches had been reviewed in time.

Your reply is the kind of message that makes me wonder why I try to
contibute to gcc at all.

Gr.
Steven

5/7/06, Steven Bosscher <stevenb.gcc (AT) gmail (DOT) comwrote:
I have bootstrapped (multilib) the
patches on x86_64 for c,c++,objc,fortran,ada,java and I'm waiting for
the regression tests to complete. Then I'll post the updated patch
with the test results (which should IMVH be a requirement FWIW).
--
There have been a significant number of changes
in the last few months, and it seems only reasonable to ensure that
significant changes in stage3 apply cleanly and don't cause regressions.

It would have been reasonable to don't let a patch go without any
reply whatsoever for four months, too.

The updates are a mere 3 changes that affect these two patches:
- CLEANUP_PRE_LP is gone
- execute in the tree_pass structure now is unsigned int
- validate_change_maybe_volatile is gone so that one hunk doesn't apply
--
I'm not sure why Paolo isn't pinging these patches himself,

Paolo _has_ pinged his patches. Many times. And besides, I'm also an
author of this new pass, so they are my patches too.
>
>This is one reason why PINGs
by interested third parties (other than the original contributor)
carry slightly less weight.

RTFfwprop.c file and see my name on it so I don't consider myself a
third party, thank you very much.
--
These submissions claim to be mostly performance neutral on SPEC, with a
slight hit for PPC,

No slight hit on PPC. I appreciate you reply to my ping, but it would
have been helpful if you've read Paolo's complete story. The PPC
regression was fixed already even before these fwprop patches were
posted.

Personally, I'd love to see the 3% compile-time improvements from
disabling path-following in CSE provided that the patch is safe and
the performance impact on most platforms is negligible. However,
there are already complaints about the reasoning used to accept/reject
significant changes in stage3, and whether they are truly ready for
GCC just becuase they are pinged continually.

First of all, this work was finished in september last year.

Second, it has been on a branch (the dataflow branch) for months now,
which has been tested on a large number of targets.

Third, this is not a stage3 contribution, and I can't help that the
reviewers don't do their job.

If I post a patch in stage2, and early stage2 too if I may add, then
you should not slap me with "significant changes in stage3", but
instead ask yourself "why didn't I take the responsibility that I have
as a reviewer to comment on the patch?". IMH you've forfeited your
right to complain about timing if a patch has been pending as long as
these two.

The postings for these patches also reported that these changes had
only been bootstrapped and regression tested on i686-pc-linux-gnu.
If someone was serious about their patches, they'd perhaps do slightly
more than the required minimum testing.

But they had previously been bootstrapped and tested by me on
{i686,x86_64,ppc,ppc64,ia64}-suse-linux-gnu. Plus SPEC on the two
x86* targets. Plus nullstone on x86*. Plus PPC testing by David
Edelsohn. Plus on a number of targets on the dataflow-branch. That
_is_ slightly more than the required minimum testing.

I realize you're not a mind reader, but when this patch was posted,
just testing on one target was enough for a patch contribution. Now
you are raising the bar because, essentially, you and your fellow
reviewers have failed. That is the way I see it at least.

There's also the issue of breakage and fallout. Paolo's most recent
patch is still causing mainline regressions on x86_64, see the post
and notice
the "PING^n fwprop" in the title.

If you had read beyond the subject, you'd have seen that the
regressions mentioned concern a reload change, which also had to be
pinged because nobody is reviewing patches. That message you point to
had nothing to do with fwprop other than the subject line.

btw, I seem to recall that all your patches in the tree overflow
mess also have caused numerous regressions. The difference between
yours and fwprops (if any, and I doubt it) is that you can approve
your own patches so that they are commited when they are ready, and
other people like Paolo and me have to wait until some reviewer is so
kind to take a little time to look at some patch. Which obviously
doesn't happen enough as proven by the huge number of pending patches
in e.g. the patch tracker. So you have time to fix whatever you've
broken, and we don't.

In fact, I'm fed up with your I-know-better attitude here. Many of
the points you bring up have little or no relation to fwprop as far as
I can see, and none of them would have been particularly relevant if
these patches had been reviewed in time.

Your reply is the kind of message that makes me wonder why I try to
contibute to gcc at all.

Gr.
Steven

Here they are.
Testresults are here:

No new regressions compared to unpatched.

Gr.
Steven

I want to stay out of any battles over this patch. Clearly it should
have been reviewed long ago, and clearly the reviewers as a whole,
including myself, are at fault.

I agree with Mark that we should be prepared to accept this patch even
in stage 3 since it was submitted before entering stage 3. That has
always been my understanding of the general rule.

Ian

+/* Return a pointer to one of the occurrences of FIND in *PX. */
+
+rtx *
+find_occurrence (rtx *px, rtx find)

I think it would be preferable to write this in terms for
for_each_rtx, unless there is some reason not to. Every time we have
a list like this:

+ case REG:
+ case CNST_INT:
+ case CNST_DUBLE:
+ case CNST_VECTR:
+ case SYMBL_REF:
+ case CDE_LABEL:
+ case PC:
+ case CC0:

we have something that needs to be updated from here until eternity.
It's true that count_occurrence is not written using for_each_rtx, but
that is a bug, not a feature.

@@ -2847,10 +2909,16 @@ auto_inc_p (rtx x)
int
loc_mentioned_in_p (rtx *loc, rtx in)
{
- enum rtx_code code = GET_CDE (in);
- const char *fmt = GET_RTX_FRMAT (code);
+ enum rtx_code code;
+ const char *fmt;
int i, j;

+ /* Allow this function to work in EXPR_LISTs. */
+ if (!in)
+ return 0;
+
+ code = GET_CDE (in);
+ fmt = GET_RTX_FRMAT (code);

No need for the comment would take it out. The code is
self-evidently reasonable, and the inline comment is distracting.

Index: df-core.c

df-core.c(revision 113581)
df-core.c(working copy)
@@ -919,7 +919,11 @@ df_bb_regno_last_use_find (struct df *df
FR_BB_INSNS_REVERSE (bb, insn)
{
unsigned int uid = INSN_UID (insn);
- for (use = DF_INSN_UID_GET (df, uid)->uses; use; use = use->next_ref)
+ struct df_insn_info *insn_info = DF_INSN_UID_GET (df, uid);
+ if (!insn_info)
+continue;
+
+ for (use = insn_info->uses; use; use = use->next_ref)
if (DF_REF_REGN (use) == regno)
return use;
}
@@ -938,7 +942,11 @@ df_bb_regno_first_def_find (struct df *d
FR_BB_INSNS (bb, insn)
{
unsigned int uid = INSN_UID (insn);
- for (def = DF_INSN_UID_GET (df, uid)->defs; def; def = def->next_ref)
+ struct df_insn_info *insn_info = DF_INSN_UID_GET (df, uid);
+ if (!insn_info)
+continue;
+
+ for (def = insn_info->defs; def; def = def->next_ref)
if (DF_REF_REGN (def) == regno)
return def;
}
@@ -957,8 +965,11 @@ df_bb_regno_last_def_find (struct df *df
FR_BB_INSNS_REVERSE (bb, insn)
{
unsigned int uid = INSN_UID (insn);
+ struct df_insn_info *insn_info = DF_INSN_UID_GET (df, uid);
+ if (!insn_info)
+continue;
- for (def = DF_INSN_UID_GET (df, uid)->defs; def; def = def->next_ref)
+ for (def = insn_info->defs; def; def = def->next_ref)
if (DF_REF_REGN (def) == regno)
return def;
}

I count six uses of DF_INSN_UID_GET in df-core.c. You are fixing
three of them. Is there any reason not to fix the other three?

There should be a blank line between the declaration of insn_info and
the test of insn_info.

In general this patch appears to be independent of the rest of the
code. Can you separate this part out and run it past Ken Zadeck and
Dan Berlin? I don't know enough to know whether this is fixing a bug
in the DF code or a bug in how the DF code is being used. I note that
the code on dataflow-branch still assumes that DF_INSN_UID_GET is
always non-NULL.

+static bool
+can_simplify_addr (rtx addr)
+{
+ rtx reg;
+
+ if (GET_CDE (addr) == PLUS)
+ reg = XEXP (addr, 0);
+ else
+ reg = addr;
+
+ return !REG_P (reg)
+ || (REGN (reg) FIRST_PSEUDREGISTER
+ && REGN (reg) != FRAME_PINTER_REGNUM
+ && REGN (reg) != HARD_FRAME_PINTER_REGNUM
+ && REGN (reg) != ARG_PINTER_REGNUM);
+}

This can't be right. FRAME_PINTER_REGNUM, HARD_FRAME_PINTER_REGNUM,
and ARG_PINTER_REGNUM are always < FIRST_PSEUDREGISTER.

The test against FIRST_PSEUDREGISTER should be >=, not >. Better
would be to use HARD_REGISTER_P.

There should be a parenthesis around the whole expression.

Perhaps this is intended to be
return (!REG_P (reg)
|| (!HARD_REGISTER_P (reg)
|| (REGN (reg) != FRAME_PINTER_REGNUM
&& REGN (reg) != HARD_FRAME_PINTER_REGNUM
&& REGN (reg) != ARG_PINTER_REGNUM)));
which can be simplified to
return (!REG_P (reg)
|| (REGN (reg) != FRAME_PINTER_REGNUM
&& REGN (reg) != HARD_FRAME_PINTER_REGNUM
&& REGN (reg) != ARG_PINTER_REGNUM));

course such a change would have to be retested.

In general this looks like an attempt to rewrite the test near the
start of find_best_addr in cse.c. If that is the case, why is it K
to not test CNSTANT_ADDRESS_P?

+/* Returns a canonical version of X for the address, from the point of view,
+ that all multiplications are represented as MULT instead of the multiply
+ by a power of 2 being represented as ASHIFT.
+
+ Every ASHIFT we find has been made by simplify_gen_binary and was not
+ there before, so it is not shared. So we can do this in place. */

How can you be sure of that? Doesn't that assume that no target will
use ASHIFT when representing an address?

+/* Inside INSN, the expression rooted at *LC has been changed, moving some
+ uses from RIG_USES. Find those that are present, and create new items
+ in the data flow object of the pass. Mark any new uses as having the
+ given TYPE. */
+static void
+update_df (rtx insn, rtx *loc, struct df_ref *orig_uses, enum df_ref_type type,
+ int new_flags)
+{
+ struct df_ref *use;
+
+ /* Else, add a use for the registers that were propagated. */

Why "Else"?

+/* Try substituting NEW into LC, which originated from forward propagation
+ of USE's value from NEW_DEF_INSN. SET_REG_EQUAL says whether we are
+ substituting the whole SET_SRC, so we can set a REG_EQUAL note if the
+ new insn is not recognized. Return whether the substitution was
+ performed. */
+
+static bool
+subst (struct df_ref *use, rtx *loc, rtx new, rtx def_insn, bool set_reg_equal)

I think the name "subst" is too generic, and in particular combine.c
has a function named "subst". Please use a different name.

So, please split out the df-core.c changes, and please make the
changes above (or explain why they should not be made) and repost.

Thanks, and sorry again for how long it has taken to look at these
patches.

Ian

Steven Bosscher wrote:

>These submissions claim to be mostly performance neutral on SPEC, with a
>slight hit for PPC,

No slight hit on PPC. I appreciate you reply to my ping, but it would
have been helpful if you've read Paolo's complete story. The PPC
regression was fixed already even before these fwprop patches were
posted.

Correct me if I'm wrong, but the only actual numbers for the final patch
I recall seeing were for ix86. I asked for more (after all we're
deleting optimization code here and should make sure there aren't any
serious regressions), but I don't think anything was ever posted.
Providing numbers would be more useful than pinging the extra n times.

Bernd

Bernd Schmidt writes:

Correct me if I'm wrong, but the only actual numbers for the final patch
I recall seeing were for ix86. I asked for more (after all we're
deleting optimization code here and should make sure there aren't any
serious regressions), but I don't think anything was ever posted.
Providing numbers would be more useful than pinging the extra n times.

You're wrong. I commented on PowerPC SPEC results a long time
ago:

This and other GCC 4.2 Project patches should have been handled in
a much more timely manner.

David

David Edelsohn wrote:
Bernd Schmidt writes:

>Correct me if I'm wrong, but the only actual numbers for the final patch
>I recall seeing were for ix86. I asked for more (after all we're
>deleting optimization code here and should make sure there aren't any
>serious regressions), but I don't think anything was ever posted.
>Providing numbers would be more useful than pinging the extra n times.

You're wrong. I commented on PowerPC SPEC results a long time
ago:

Notice how I said "actual numbers", and "final patch"? And if you ran
the tests against the latest patch, you might as well post the results.

This and other GCC 4.2 Project patches should have been handled in
a much more timely manner.

Quite possibly. I was prepared to review/approve the fwprop stuff much
earlier, but got little reaction to my request for data, so I decided
not to spend more time for the moment.

Bernd

Bernd Schmidt writes:
BerndNotice how I said "actual numbers", and "final patch"? And if you ran
Berndthe tests against the latest patch, you might as well post the results.
I cannot post numbers, as you should be well aware.
David

David Edelsohn wrote:
BerndNotice how I said "actual numbers", and "final patch"? And if you ran
Berndthe tests against the latest patch, you might as well post the results.
I cannot post numbers, as you should be well aware.
?
Bernd

Bernd Schmidt writes:

>I cannot post numbers, as you should be well aware.

Bernd?

First, SPEC has rules. Even if I qualify everything with
"non-reportable", computer hardware vendors do not want employees posting
numbers.

I do not believe that a table of 0%, 1%, 2% provides much more
information than my statement: "the differences were in the noise or
better with the fwprop patch"

If you wanted more information when I replied on the thread in
March, you certainly provided no feedback at the time.

If you are inclined to approve the patch, then I request that you
go ahead and approve the patch.

David

David Edelsohn wrote:
Bernd Schmidt writes:

I cannot post numbers, as you should be well aware.

Bernd?

First, SPEC has rules.

Those don't seem to stop people setting up nightly SPEC testers. There
are alternative benchmarks too.

Even if I qualify everything with
"non-reportable", computer hardware vendors do not want employees posting
numbers.

D'oh; oh well. But there must be other people reading this list who
have interesting hardware they can run this on? Diego runs PPC SPEC
tests, Michael Matz has an ia64 tester; did anyone ask them for assistance?

I do not believe that a table of 0%, 1%, 2% provides much more
information than my statement: "the differences were in the noise or
better with the fwprop patch"

There's a difference between hard data and anecdotal evidence.

If you wanted more information when I replied on the thread in
March, you certainly provided no feedback at the time.

I thought I had been quite clear about what I wanted in an earlier post,
but yes, I could have repeated myself.

If you are inclined to approve the patch, then I request that you
go ahead and approve the patch.

I read through it once, decided it looked quite sensible, and asked for
more data - I'd still want to go through it more carefully, but it makes
little sense to do so if there are serious performance problems. I
don't expect there to be any, but I'd like to be sure before giving the
go-ahead to remove a set of optimizations we've had for a long time in
favour of another. But I have a feeling I'm repeating myself.

As far as I'm concerned, you can either provide what I asked for, which
I do not think is unreasonable for a patch of this magnitude, or you can
ignore my requests and just keep pinging in the hope that another
reviewer picks it up. In the latter case, please refrain from
complaining about lack of reviewer attention. I notice that Roger asked
for pretty much the same things.

Maybe the pings should have included requests for testers?

Bernd

07 May 2006 13:04:40 -0700, Ian Lance Taylor <ian (AT) airs (DOT) comwrote:
+/* Return a pointer to one of the occurrences of FIND in *PX. */
+
+rtx *
+find_occurrence (rtx *px, rtx find)

I think it would be preferable to write this in terms for
for_each_rtx, unless there is some reason not to.

IIRC the reason for not using for_each_rtx was that we can't return a
pointer to FIND. Suggestions for doing the same thing with
for_each_rtx are welcome, but I couldn't think of a way to achieve the
same thing with for_each_rtx :-(

I count six uses of DF_INSN_UID_GET in df-core.c. You are fixing
three of them. Is there any reason not to fix the other three?

Actually I think that we shouldn't have to fix any of them, or at
least not this way. What happens is that we segfault in this loop in
e.g. df_bb_regno_last_def_find

FR_BB_INSNS_REVERSE (bb, insn)
{
unsigned int uid = INSN_UID (insn);

for (def = DF_INSN_UID_GET (df, uid)->defs; def; def = def->next_ref)
if (DF_REF_REGN (def) == regno)
return def;
}

If we do not find a DEF setting REGN, we end up looking for
DF_INSN_UID_GET on a NTE:

Breakpoint 3, df_bb_regno_last_def_find (df=0xc7b140,
bb=0x2aaaaafac300, regno=62)
at df-core.c:957
957 FR_BB_INSNS_REVERSE (bb, insn)
(gdb) p debug_bb(bb)
;; basic block 42, loop depth 2, count 0
;; prev block 38, next block 39
;; pred: 38 [100.0%] (fallthru) 3 [100.0%]
;; succ: 4 [97.5%] 39 [2.5%] (fallthru,loop_exit)
;; Registers live at start: (nil)
(code_label 261 225 260 42 38 "" [1 uses])
(note 260 261 227 42 [bb 42] NTE_INSN_BASIC_BLCK)
(insn 227 260 228 42 (set (reg:QI 70 [ D.1933 ])
(mem:QI (reg/v/f:DI 62 [ s.48 ]) [0 S1 A8])) 55 {*movqi_1} (nil)
(nil))
(insn 228 227 229 42 (set (reg:CCZ 17 flags)
(compare:CCZ (reg:QI 70 [ D.1933 ])
(const_int 0 [0x0]))) 9 {*cmpqi_ccno_1} (nil)
(nil))
(jump_insn 229 228 231 42 (set (pc)
(if_then_else (ne (reg:CCZ 17 flags)
(const_int 0 [0x0]))
(label_ref 23)
(pc))) 511 {*jcc_1} (nil)
(expr_list:REG_BR_PRB (const_int 9750 [0x2616])
(nil)))
;; Registers live at end: (nil)
$13 = void
(gdb) p regno
$14 = 62
(gdb) cont
Continuing.

Program received signal SIGSEGV, Segmentation fault.
0x0000000000575927 in df_bb_regno_last_def_find (df=0xc7b140,
bb=<value optimized out>,
regno=62) at df-core.c:961
961 for (def = DF_INSN_UID_GET (df, uid)->defs; def; def =
def->next_ref)
(gdb)

Dan, what do you think about this? Should we make sure in
df_bb_regno_last_def_find that we are looking at insns, i.e. something
like this:

FR_BB_INSNS_REVERSE (bb, insn)
{
- unsigned int uid = INSN_UID (insn);
+ unsigned int uid;

+ if (! INSN_P (insn))
+ continue;
+
+ uid = INSN_UID (insn);
for (def = DF_INSN_UID_GET (df, uid)->defs; def; def = def->next_ref)
if (DF_REF_REGN (def) == regno)
return def;
}

It seems to me that getting DF_INSN_UID_GET on a NTE is always wrong?

I note that
the code on dataflow-branch still assumes that DF_INSN_UID_GET is
always non-NULL.

Ehm, yes
I also note that fwprop is not enabled by default on the dataflow branch :-/

+/* Returns a canonical version of X for the address, from the point of view,
+ that all multiplications are represented as MULT instead of the multiply
+ by a power of 2 being represented as ASHIFT.
+
+ Every ASHIFT we find has been made by simplify_gen_binary and was not
+ there before, so it is not shared. So we can do this in place. */

How can you be sure of that? Doesn't that assume that no target will
use ASHIFT when representing an address?

Using ASHIFT in an address is non-canonical RTL. That means we can
make that assumption, no?

(And, yes, that's actually a problem for targets like e.g. ARM, which
has instructions for manipulating addresses with SHIFTs instead of
MULs ;-).

Thanks for your review. I'm still looking at/working on the other
issues you mentioned.

Gr.
Steven

Steven Bosscher wrote:
07 May 2006 13:04:40 -0700, Ian Lance Taylor <ian (AT) airs (DOT) comwrote:
+/* Return a pointer to one of the occurrences of FIND in *PX. */
+
+rtx *
+find_occurrence (rtx *px, rtx find)
>I think it would be preferable to write this in terms for
>for_each_rtx, unless there is some reason not to.

IIRC the reason for not using for_each_rtx was that we can't return a
pointer to FIND. Suggestions for doing the same thing with
for_each_rtx are welcome, but I couldn't think of a way to achieve the
same thing with for_each_rtx :-(

>I count six uses of DF_INSN_UID_GET in df-core.c. You are fixing
>three of them. Is there any reason not to fix the other three?

Actually I think that we shouldn't have to fix any of them, or at
least not this way. What happens is that we segfault in this loop in
e.g. df_bb_regno_last_def_find

FR_BB_INSNS_REVERSE (bb, insn)
{
unsigned int uid = INSN_UID (insn);

for (def = DF_INSN_UID_GET (df, uid)->defs; def; def = def->next_ref)
if (DF_REF_REGN (def) == regno)
return def;
}

If we do not find a DEF setting REGN, we end up looking for
DF_INSN_UID_GET on a NTE:

Breakpoint 3, df_bb_regno_last_def_find (df=0xc7b140,
bb=0x2aaaaafac300, regno=62)
at df-core.c:957
957 FR_BB_INSNS_REVERSE (bb, insn)
(gdb) p debug_bb(bb)
;; basic block 42, loop depth 2, count 0
;; prev block 38, next block 39
;; pred: 38 [100.0%] (fallthru) 3 [100.0%]
;; succ: 4 [97.5%] 39 [2.5%] (fallthru,loop_exit)
;; Registers live at start: (nil)
(code_label 261 225 260 42 38 "" [1 uses])
(note 260 261 227 42 [bb 42] NTE_INSN_BASIC_BLCK)
(insn 227 260 228 42 (set (reg:QI 70 [ D.1933 ])
(mem:QI (reg/v/f:DI 62 [ s.48 ]) [0 S1 A8])) 55 {*movqi_1} (nil)
(nil))
(insn 228 227 229 42 (set (reg:CCZ 17 flags)
(compare:CCZ (reg:QI 70 [ D.1933 ])
(const_int 0 [0x0]))) 9 {*cmpqi_ccno_1} (nil)
(nil))
(jump_insn 229 228 231 42 (set (pc)
(if_then_else (ne (reg:CCZ 17 flags)
(const_int 0 [0x0]))
(label_ref 23)
(pc))) 511 {*jcc_1} (nil)
(expr_list:REG_BR_PRB (const_int 9750 [0x2616])
(nil)))
;; Registers live at end: (nil)
$13 = void
(gdb) p regno
$14 = 62
(gdb) cont
Continuing.

Program received signal SIGSEGV, Segmentation fault.
0x0000000000575927 in df_bb_regno_last_def_find (df=0xc7b140,
bb=<value optimized out>,
regno=62) at df-core.c:961
961 for (def = DF_INSN_UID_GET (df, uid)->defs; def; def =
def->next_ref)
(gdb)

Dan, what do you think about this? Should we make sure in
df_bb_regno_last_def_find that we are looking at insns, i.e. something
like this:

FR_BB_INSNS_REVERSE (bb, insn)
{
- unsigned int uid = INSN_UID (insn);
+ unsigned int uid;

+ if (! INSN_P (insn))
+ continue;
+
+ uid = INSN_UID (insn);
for (def = DF_INSN_UID_GET (df, uid)->defs; def; def = def->next_ref)
if (DF_REF_REGN (def) == regno)
return def;
}

It seems to me that getting DF_INSN_UID_GET on a NTE is always wrong?

It is. The above should work.

Sunday 07 May 2006 22:04, Ian Lance Taylor wrote:
+/* Return a pointer to one of the occurrences of FIND in *PX. */
+
+rtx *
+find_occurrence (rtx *px, rtx find)

I think it would be preferable to write this in terms for
for_each_rtx, unless there is some reason not to.

As I said in a previous mail, I don't think we can do this because
we need to have the address of an rtx returned and I don't see how
we can do that with for_each_rtx.

+ /* Allow this function to work in EXPR_LISTs. */

No need for the comment would take it out.

Done.

I count six uses of DF_INSN_UID_GET in df-core.c. You are fixing
three of them. Is there any reason not to fix the other three?

I've looked at this df-core.c change and found that we should have
a different fix. That fix is now already in mainline.

FWIW we only had to fix three because those are the only three places
where we walk an insn chain (using FR_BB_INSN*).

+static bool
+can_simplify_addr (rtx addr)
+{
+ rtx reg;
+
+ if (GET_CDE (addr) == PLUS)
+ reg = XEXP (addr, 0);
+ else
+ reg = addr;
+
+ return !REG_P (reg)
+ || (REGN (reg) FIRST_PSEUDREGISTER
+ && REGN (reg) != FRAME_PINTER_REGNUM
+ && REGN (reg) != HARD_FRAME_PINTER_REGNUM
+ && REGN (reg) != ARG_PINTER_REGNUM);
+}

This can't be right. FRAME_PINTER_REGNUM, HARD_FRAME_PINTER_REGNUM,
and ARG_PINTER_REGNUM are always < FIRST_PSEUDREGISTER.

The test against FIRST_PSEUDREGISTER should be >=, not >. Better
would be to use HARD_REGISTER_P.

There should be a parenthesis around the whole expression.

Done.

Perhaps this is intended to be
return (!REG_P (reg)

|| (!HARD_REGISTER_P (reg)
||
|| (REGN (reg) != FRAME_PINTER_REGNUM

&& REGN (reg) != HARD_FRAME_PINTER_REGNUM
&& REGN (reg) != ARG_PINTER_REGNUM)));
which can be simplified to
return (!REG_P (reg)

|| (REGN (reg) != FRAME_PINTER_REGNUM

&& REGN (reg) != HARD_FRAME_PINTER_REGNUM
&& REGN (reg) != ARG_PINTER_REGNUM));

course such a change would have to be retested.

In general this looks like an attempt to rewrite the test near the
start of find_best_addr in cse.c. If that is the case, why is it K
to not test CNSTANT_ADDRESS_P?

I believe we should test CNSTANT_ADDRESS_P too. We can't simplify a
CNSTANT_ADDRESS_P address if doc/tm.texi is right.

So that whole function now looks like this:

static bool
can_simplify_addr (rtx addr)
{
rtx reg;

if (CNSTANT_ADDRESS_P (addr))
return false;

if (GET_CDE (addr) == PLUS)
reg = XEXP (addr, 0);
else
reg = addr;

return (!REG_P (reg)
|| (REGN (reg) != FRAME_PINTER_REGNUM
&& REGN (reg) != HARD_FRAME_PINTER_REGNUM
&& REGN (reg) != ARG_PINTER_REGNUM));
}

+/* Returns a canonical version of X for the address, from the point of
view, + that all multiplications are represented as MULT instead of the
multiply + by a power of 2 being represented as ASHIFT.
+
+ Every ASHIFT we find has been made by simplify_gen_binary and was not
+ there before, so it is not shared. So we can do this in place. */

How can you be sure of that? Doesn't that assume that no target will
use ASHIFT when representing an address?

The assumption was that ASHIFT in an address is not canonical. However,
cse.c:canon_for_address uses gen_rtx_MULT. I have no strong feelings
either way, but modifying in place works. Generating a new RTX is a bit
cleaner perhaps. I'll change this if you think that is better. For now
I haven't changed it.

+/* Inside INSN, the expression rooted at *LC has been changed, moving
some + uses from RIG_USES. Find those that are present, and create
new items + in the data flow object of the pass. Mark any new uses as
having the + given TYPE. */
+static void
+update_df (rtx insn, rtx *loc, struct df_ref *orig_uses, enum
df_ref_type type, + int new_flags)
+{
+ struct df_ref *use;
+
+ /* Else, add a use for the registers that were propagated. */

Why "Else"?

Typo. Fixed.

+/* Try substituting NEW into LC, which originated from forward
propagation + of USE's value from NEW_DEF_INSN. SET_REG_EQUAL says
whether we are + substituting the whole SET_SRC, so we can set a
REG_EQUAL note if the + new insn is not recognized. Return whether the
substitution was + performed. */
+
+static bool
+subst (struct df_ref *use, rtx *loc, rtx new, rtx def_insn, bool
set_reg_equal)

I think the name "subst" is too generic, and in particular combine.c
has a function named "subst". Please use a different name.

It is now called try_fwprop_subst.

So this is the first part of the changes. The second bits that clean
up cse.c are unchanged. I have only tested this with a C bootstrap on
x86-linux, but I'll give it a full run tomorrow and if it passes, I'll
post it as an RFT. I'm posting this now because if there's anything
you spot right away, I can make the changes _before_ spending another
full day of computer cycles on testing ;-)

Gr.
Steven

* Makefile.in: Add fwprop.o.
* common.opt (-fforward-propagate): New.
* fwprop.c: New file.
* opts.c (decode_options): Don't enable CSE path following by default.
Enable forward propagation at
* passes.c (init_optimization_passes): Schedule forward propagation.
* rtl.h (find_occurrence): Declare.
* rtlanal.c (find_occurrence): New.
(loc_mentioned_in_p): Support NULL value of the second parameter.
* timevar.def (TV_FWPRP): New.
* tree-pass.h (pass_rtl_fwprop, pass_rtl_fwprop_with_addr): New.

* doc/invoke.texi ( ): Document fwprop.

Index: doc/invoke.texi

doc/invoke.texi(revision 113736)
doc/invoke.texi(working copy)
@@ -310,7 +310,7 @@ C and C++ Dialects}.
-fcse-skip-blocks -fcx-limited-range -fdata-sections @gol
-fdelayed-branch -fdelete-null-pointer-checks -fearly-inlining @gol
-fexpensive-optimizations -ffast-math -ffloat-store @gol
-ffunction-sections @gol
+-fforce-addr -fforward-propagate -ffunction-sections @gol
-fgcse -fgcse-lm -fgcse-sm -fgcse-las -fgcse-after-reload @gol
-fcrossjumping -fif-conversion -fif-conversion2 @gol
-finline-functions -finline-functions-called-once @gol
@@ -4565,6 +4565,16 @@ register-load. This option is now a nop
Force memory address constants to be copied into registers before
doing arithmetic on them.

+@item -fforward-propagate
+@opindex fforward-propagate
+Perform a forward propagation pass on RTL, which try to combine two
+instructions and see if the result can be simplified. If loop unrolling
+is active, two passes are performed and the second is scheduled after
+loop unrolling.
+
+This option is enabled by default at optimization levels @option{},
+@option{}, @option{}.
+
@item -fomit-frame-pointer
@opindex fomit-frame-pointer
Don't keep the frame pointer in a register for functions that
Index: tree-pass.h

tree-pass.h(revision 113736)
tree-pass.h(working copy)
@@ -330,6 +330,8 @@ extern struct tree_opt_pass pass_rtl_eh;
extern struct tree_opt_pass pass_initial_value_sets;
extern struct tree_opt_pass pass_unshare_all_rtl;
extern struct tree_opt_pass pass_instantiate_virtual_regs;
+extern struct tree_opt_pass pass_rtl_fwprop;
+extern struct tree_opt_pass pass_rtl_fwprop_addr;
extern struct tree_opt_pass pass_jump2;
extern struct tree_opt_pass pass_cse;
extern struct tree_opt_pass pass_gcse;
Index: rtlanal.c

rtlanal.c(revision 113736)
rtlanal.c(working copy)
@@ -552,6 +552,68 @@ count_occurrences (rtx x, rtx find, int
}
return count;
}
+
+/* Return a pointer to one of the occurrences of FIND in *PX. */
+
+rtx *
+find_occurrence (rtx *px, rtx find)
+{
+ int i, j;
+ enum rtx_code code;
+ const char *format_ptr;
+ rtx x = *px;
+
+ if (x == find)
+ return px;
+
+ code = GET_CDE (x);
+ switch (code)
+ {
+ case REG:
+ case CNST_INT:
+ case CNST_DUBLE:
+ case CNST_VECTR:
+ case SYMBL_REF:
+ case CDE_LABEL:
+ case PC:
+ case CC0:
+ return NULL;
+
+ case MEM:
+ if (MEM_P (find) && rtx_equal_p (x, find))
+return px;
+ break;
+
+ default:
+ break;
+ }
+
+ format_ptr = GET_RTX_FRMAT (code);
+
+ for (i = 0; i < GET_RTX_LENGTH (code); i++)
+ {
+ rtx *result;
+ switch (*format_ptr++)
+{
+case 'e':
+ result = find_occurrence (&XEXP (x, i), find);
+ if (result)
+ return result;
+ break;
+
+case 'E':
+ for (j = 0; j < XVECLEN (x, i); j++)
+ {
+ result = find_occurrence (&XVECEXP (x, i, j), find);
+ if (result)
+ return result;
+ }
+ break;
+}
+ }
+
+ return 0;
+}

/* Nonzero if register REG appears somewhere within IN.
Also works if REG is not a register; in this case it checks
@@ -2847,10 +2909,15 @@ auto_inc_p (rtx x)
int
loc_mentioned_in_p (rtx *loc, rtx in)
{
- enum rtx_code code = GET_CDE (in);
- const char *fmt = GET_RTX_FRMAT (code);
+ enum rtx_code code;
+ const char *fmt;
int i, j;

+ if (!in)
+ return 0;
+
+ code = GET_CDE (in);
+ fmt = GET_RTX_FRMAT (code);
for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
{
if (loc == &in->u.fld[i].rt_rtx)
Index: opts.c

opts.c(revision 113736)
opts.c(working copy)
@@ -559,8 +559,7 @@ decode_options (unsigned int argc, const
flag_thread_jumps = 1;
flag_crossjumping = 1;
flag_optimize_sibling_calls = 1;
- flag_cse_follow_jumps = 1;
- flag_cse_skip_blocks = 1;
+ flag_forward_propagate = 1;
flag_gcse = 1;
flag_expensive_optimizations = 1;
flag_ipa_type_escape = 1;
Index: timevar.def

timevar.def(revision 113736)
timevar.def(working copy)
@@ -128,6 +128,7 @@ DEFTIMEVAR (TV_TEMPLATE_INSTANTIATIN, "
DEFTIMEVAR (TV_EXPAND , "expand")
DEFTIMEVAR (TV_VARCNST , "varconst")
DEFTIMEVAR (TV_JUMP , "jump")
+DEFTIMEVAR (TV_FWPRP , "forward prop")
DEFTIMEVAR (TV_CSE , "CSE")
DEFTIMEVAR (TV_LP , "loop analysis")
DEFTIMEVAR (TV_GCSE , "global CSE")
Index: common.opt

common.opt(revision 113736)
common.opt(working copy)
@@ -440,6 +440,10 @@ fforce-mem
Common Report Var(flag_force_mem)
Copy memory operands into registers before use

+fforward-propagate
+Common Report Var(flag_forward_propagate)
+Perform a forward propagation pass on RTL
+
; Nonzero means don't put addresses of constant functions in registers.
; Used for compiling the Unix kernel, where strange substitutions are
; done on the assembly output.
Index: rtl.h

rtl.h(revision 113736)
rtl.h(working copy)
@@ -1683,6 +1683,7 @@ extern HST_WIDE_INT get_integer_term (r
extern rtx get_related_value (rtx);
extern int reg_mentioned_p (rtx, rtx);
extern int count_occurrences (rtx, rtx, int);
+extern rtx *find_occurrence (rtx *, rtx);
extern int reg_referenced_p (rtx, rtx);
extern int reg_used_between_p (rtx, rtx, rtx);
extern int reg_set_between_p (rtx, rtx, rtx);
Index: Makefile.in

Makefile.in(revision 113736)
Makefile.in(working copy)
@@ -983,7 +983,7 @@ BJS-common = \
debug.o df-core.o df-problems.o df-scan.o dfp.o diagnostic.o dojump.o \
dominance.o loop-doloop.o \
dwarf2asm.o dwarf2out.o emit-rtl.o except.o explow.o loop-iv.o \
- expmed.o expr.o final.o flow.o fold-const.o function.o gcse.o \
+ expmed.o expr.o final.o flow.o fold-const.o function.o fwprop.o gcse.o \
genrtl.o ggc-common.o global.o graph.o gtype-desc.o \
haifa-sched.o hooks.o ifcvt.o insn-attrtab.o insn-emit.o insn-modes.o \
insn-extract.o insn-opinit.o insn-output.o insn-peep.o insn-recog.o \
@@ -2311,12 +2311,15 @@ cse.o : cse.c $(CNFIG_H) $(SYSTEM_H) co
hard-reg-set.h $(FLAGS_H) insn-config.h $(RECG_H) $(EXPR_H) toplev.h \
output.h $(FUNCTIN_H) $(BASIC_BLCK_H) $(GGC_H) $(TM_P_H) $(TIMEVAR_H) \
except.h $(TARGET_H) $(PARAMS_H) rtlhooks-def.h tree-pass.h $(REAL_H)
+fwprop.o : fwprop.c $(CNFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(RTL_H) \
+ toplev.h insn-config.h $(RECG_H) $(FLAGS_H) $(BSTACK_H) $(BASIC_BLCK_H) \
+ output.h $(DF_H) alloc-pool.h $(TIMEVAR_H) tree-pass.h
web.o : web.c $(CNFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(RTL_H) \
hard-reg-set.h $(FLAGS_H) $(BASIC_BLCK_H) $(FUNCTIN_H) output.h toplev.h \
$(DF_H) $(BSTACK_H) timevar.h tree-pass.h
see.o : see.c $(CNFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(RTL_H) \
hard-reg-set.h $(FLAGS_H) $(BASIC_BLCK_H) function.h output.h toplev.h \
- $(DF_H) $(BSTACK_H) timevar.h tree-pass.h
+ $(DF_H) $(BSTACK_H) $(TIMEVAR_H) tree-pass.h
gcse.o : gcse.c $(CNFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(RTL_H) \
$(REGS_H) hard-reg-set.h $(FLAGS_H) $(REAL_H) insn-config.h $(GGC_H) \
$(RECG_H) $(EXPR_H) $(BASIC_BLCK_H) $(FUNCTIN_H) output.h toplev.h \
Index: passes.c

passes.c(revision 113736)
passes.c(working copy)
@@ -635,6 +635,7 @@ init_optimization_passes (void)
NEXT_PASS (pass_instantiate_virtual_regs);
NEXT_PASS (pass_jump2);
NEXT_PASS (pass_cse);
+ NEXT_PASS (pass_rtl_fwprop);
NEXT_PASS (pass_gcse);
NEXT_PASS (pass_jump_bypass);
NEXT_PASS (pass_rtl_ifcvt);
@@ -645,6 +646,7 @@ init_optimization_passes (void)
NEXT_PASS (pass_loop2);
NEXT_PASS (pass_web);
NEXT_PASS (pass_cse2);
+ NEXT_PASS (pass_rtl_fwprop_addr);
NEXT_PASS (pass_life);
NEXT_PASS (pass_combine);
NEXT_PASS (pass_if_after_combine);
Index: fwprop.c

fwprop.c(revision 0)
fwprop.c(revision 0)
@@ -0,0 +1,976 @@
+/* RTL-based forward propagation pass for GNU compiler.
+ Copyright (C) 2005, 2006 Free Software Foundation, Inc.
+ Contributed by Paolo Bonzini and Steven Bosscher.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 2, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FR A PARTICULAR PURPSE. See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file CPYING. If not, write to the Free
+Software Foundation, 59 Temple Place - Suite 330, Boston, MA
+02111-1307, USA. */
+
+#include "config.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tm.h"
+#include "toplev.h"
+
+#include "timevar.h"
+#include "rtl.h"
+#include "tm_p.h"
+#include "emit-rtl.h"
+#include "insn-config.h"
+#include "recog.h"
+#include "flags.h"
+#include "obstack.h"
+#include "basic-block.h"
+#include "output.h"
+#include "df.h"
+#include "target.h"
+#include "cfgloop.h"
+#include "tree-pass.h"
+
+
+/* This pass does simple forward propagation and simplification when an
+ operand of an insn can only come from a single def. This pass uses
+ df.c, so it is global. However, we only do limited analysis of
+ available expressions.
+
+ 1) The pass tries to propagate the source of the def into the use,
+ and checks if the result is independent of the substituted value.
+ For example, the high word of a (zero_extend:DI (reg:SI M)) is always
+ zero, independent of the source register.
+
+ In particular, we propagate constants into the use site. Sometimes
+ RTL expansion did not put the constant in the same insn on purpose,
+ to satisfy a predicate, and the result will fail to be recognized;
+ but this happens rarely and in this case we can still create a
+ REG_EQUAL note. For multi-word operations, this
+
+ (set (subreg:SI (reg:DI 120) 0) (const_int 0))
+ (set (subreg:SI (reg:DI 120) 4) (const_int -1))
+ (set (subreg:SI (reg:DI 122) 0)
+ (ior:SI (subreg:SI (reg:DI 119) 0) (subreg:SI (reg:DI 120) 0)))
+ (set (subreg:SI (reg:DI 122) 4)
+ (ior:SI (subreg:SI (reg:DI 119) 4) (subreg:SI (reg:DI 120) 4)))
+
+ can be simplified to the much simpler
+
+ (set (subreg:SI (reg:DI 122) 0) (subreg:SI (reg:DI 119)))
+ (set (subreg:SI (reg:DI 122) 4) (const_int -1))
+
+ This particular propagation is also effective at putting together
+ complex addressing modes. We are more aggressive inside MEMs, in
+ that all definitions are propagated if the use is in a MEM; if the
+ result is a valid memory address we check address_cost to decide
+ whether the substitution is worthwhile.
+
+ 2) The pass propagates register copies. This is not as effective as
+ the copy propagation done by CSE's canon_reg, which works by walking
+ the instruction chain, it can help the other transformations.
+
+ We should consider removing this optimization, and instead reorder the
+ RTL passes, because GCSE does this transformation too. With some luck,
+ the CSE pass at the end of rest_of_handle_gcse could also go away.
+
+ 3) The pass looks for paradoxical subregs that are actually unnecessary.
+ Things like this:
+
+ (set (reg:QI 120) (subreg:QI (reg:SI 118) 0))
+ (set (reg:QI 121) (subreg:QI (reg:SI 119) 0))
+ (set (reg:SI 122) (plus:SI (subreg:SI (reg:QI 120) 0)
+ (subreg:SI (reg:QI 121) 0)))
+
+ are very common on machines that can only do word-sized operations.
+ For each use of a paradoxical subreg (subreg:WIDER (reg:NARRW N) 0),
+ if it has a single def and it is (subreg:NARRW (reg:WIDE M) 0),
+ we can replace the paradoxical subreg with simply (reg:WIDE M). The
+ above will simplify this to
+
+ (set (reg:QI 120) (subreg:QI (reg:SI 118) 0))
+ (set (reg:QI 121) (subreg:QI (reg:SI 119) 0))
+ (set (reg:SI 122) (plus:SI (reg:SI 118) (reg:SI 119)))
+
+ where the first two insns are now dead. */
+
+
+static struct loops loops;
+static struct df *df;
+static int num_changes;
+
+
+/* Do not try to replace constant addresses or addresses of local and
+ argument slots. These MEM expressions are made only once and inserted
+ in many instructions, as well as being used to control symbol table
+ output. It is not safe to clobber them.
+
+ There are some uncommon cases where the address is already in a register
+ for some reason, but we cannot take advantage of that because we have
+ no easy way to unshare the MEM. In addition, looking up all stack
+ addresses is costly. */
+
+static bool
+can_simplify_addr (rtx addr)
+{
+ rtx reg;
+
+ if (CNSTANT_ADDRESS_P (addr))
+ return false;
+
+ if (GET_CDE (addr) == PLUS)
+ reg = XEXP (addr, 0);
+ else
+ reg = addr;
+
+ return (!REG_P (reg)
+ || (REGN (reg) != FRAME_PINTER_REGNUM
+ && REGN (reg) != HARD_FRAME_PINTER_REGNUM
+ && REGN (reg) != ARG_PINTER_REGNUM));
+}
+
+/* Returns a canonical version of X for the address, from the point of view,
+ that all multiplications are represented as MULT instead of the multiply
+ by a power of 2 being represented as ASHIFT.
+
+ Every ASHIFT we find has been made by simplify_gen_binary and was not
+ there before, so it is not shared. So we can do this in place. */
+
+static void
+canonicalize_address (rtx x)
+{
+ for (;;)
+ switch (GET_CDE (x))
+ {
+ case ASHIFT:
+ if (GET_CDE (XEXP (x, 1)) == CNST_INT
+ && INTVAL (XEXP (x, 1)) < GET_MDE_BITSIZE (GET_MDE (x))
+ && INTVAL (XEXP (x, 1)) >= 0)
+ {
+ HST_WIDE_INT shift = INTVAL (XEXP (x, 1));
+ PUT_CDE (x, MULT);
+ XEXP (x, 1) = gen_int_mode ((HST_WIDE_INT) 1 << shift,
+GET_MDE (x));
+ }
+
+x = XEXP (x, 0);
+ break;
+
+ case PLUS:
+ if (GET_CDE (XEXP (x, 0)) == PLUS
+ || GET_CDE (XEXP (x, 0)) == ASHIFT
+ || GET_CDE (XEXP (x, 0)) == CNST)
+ canonicalize_address (XEXP (x, 0));
+
+x = XEXP (x, 1);
+ break;
+
+ case CNST:
+x = XEXP (x, 0);
+ break;
+
+ default:
+ return;
+ }
+}
+
+/* LD is a memory address. Return whether it is good to use NEW instead,
+ for a memory access in the given MDE. */
+
+static bool
+should_replace_address (rtx old, rtx new, enum machine_mode mode)
+{
+ int gain;
+
+ if (rtx_equal_p (old, new) || !memory_address_p (mode, new))
+ return false;
+
+ /* Copy propagation is always ok. */
+ if (REG_P (old) && REG_P (new))
+ return true;
+
+ /* Prefer the new address if it is less expensive. */
+ gain = address_cost (old, mode) - address_cost (new, mode);
+
+ /* If the addresses have equivalent cost, prefer the new address
+ if it has the highest `rtx_cost'. That has the potential of
+ eliminating the most insns without additional costs, and it
+ is the same that cse.c used to do. */
+ if (gain == 0)
+ gain = rtx_cost (new, SET) - rtx_cost (old, SET);
+
+ return (gain 0);
+}
+
+/* Replace all occurrences of LD in *PX with NEW and try to simplify the
+ resulting expression. Replace *PX with a new RTL expression if an
+ occurrence of LD was found.
+
+ If CAN_APPEAR is true, we always return true; if it is false, we
+ can return false if, for at least one occurrence LD, we failed to
+ collapse the result to a constant. For example, (mult:M (reg:M A)
+ (minus:M (reg:M B) (reg:M A))) may collapse to zero if replacing
+ (reg:M B) with (reg:M A).
+
+ CAN_APPEAR is disregarded inside MEMs: in that case, we always return
+ true if the simplification is a cheaper and valid memory address.
+
+ This is only a wrapper around simplify-rtx.c: do not add any pattern
+ matching code here. (The sole exception is the handling of LSUM, but
+ that is because there is no simplify_gen_* function for LSUM). */
+
+static bool
+propagate_rtx_1 (rtx *px, rtx old, rtx new, bool can_appear)
+{
+ rtx x = *px, tem = NULL_RTX, op0, op1, op2;
+ enum rtx_code code = GET_CDE (x);
+ enum machine_mode mode = GET_MDE (x);
+ enum machine_mode op_mode;
+ bool valid_ops = true;
+
+ /* If X is LD_RTX, return NEW_RTX. , if this is an expression,
+ try to build a new expression from recursive substitution. */
+
+ if (x == old)
+ {
+ *px = new;
+ return can_appear;
+ }
+
+ switch (GET_RTX_CLASS (code))
+ {
+ case RTX_UNARY:
+ op0 = XEXP (x, 0);
+ op_mode = GET_MDE (op0);
+ valid_ops &= propagate_rtx_1 (&op0, old, new, can_appear);
+ if (op0 == XEXP (x, 0))
+return true;
+ tem = simplify_gen_unary (code, mode, op0, op_mode);
+ break;
+
+ case RTX_BIN_ARITH:
+ case RTX_CMM_ARITH:
+ op0 = XEXP (x, 0);
+ op1 = XEXP (x, 1);
+ valid_ops &= propagate_rtx_1 (&op0, old, new, can_appear);
+ valid_ops &= propagate_rtx_1 (&op1, old, new, can_appear);
+ if (op0 == XEXP (x, 0) && op1 == XEXP (x, 1))
+return true;
+ tem = simplify_gen_binary (code, mode, op0, op1);
+ break;
+
+ case RTX_CMPARE:
+ case RTX_CMM_CMPARE:
+ op0 = XEXP (x, 0);
+ op1 = XEXP (x, 1);
+ op_mode = GET_MDE (op0) != VIDmode ? GET_MDE (op0) : GET_MDE (op1);
+ valid_ops &= propagate_rtx_1 (&op0, old, new, can_appear);
+ valid_ops &= propagate_rtx_1 (&op1, old, new, can_appear);
+ if (op0 == XEXP (x, 0) && op1 == XEXP (x, 1))
+return true;
+ tem = simplify_gen_relational (code, mode, op_mode, op0, op1);
+ break;
+
+ case RTX_TERNARY:
+ case RTX_BITFIELDPS:
+ op0 = XEXP (x, 0);
+ op1 = XEXP (x, 1);
+ op2 = XEXP (x, 2);
+ op_mode = GET_MDE (op0);
+ valid_ops &= propagate_rtx_1 (&op0, old, new, can_appear);
+ valid_ops &= propagate_rtx_1 (&op1, old, new, can_appear);
+ valid_ops &= propagate_rtx_1 (&op2, old, new, can_appear);
+ if (op0 == XEXP (x, 0) && op1 == XEXP (x, 1) && op2 == XEXP (x, 2))
+return true;
+ if (op_mode == VIDmode)
+op_mode = GET_MDE (op0);
+ tem = simplify_gen_ternary (code, mode, op_mode, op0, op1, op2);
+ break;
+
+ case RTX_EXTRA:
+ /* The only case we try to handle is a SUBREG. */
+ if (code == SUBREG)
+{
+ op0 = XEXP (x, 0);
+ valid_ops &= propagate_rtx_1 (&op0, old, new, can_appear);
+ if (op0 == XEXP (x, 0))
+ return true;
+ tem = simplify_gen_subreg (mode, op0, GET_MDE (SUBREG_REG (x)),
+ SUBREG_BYTE (x));
+}
+ break;
+
+ case RTXBJ:
+ if (code == MEM && x != new)
+{
+ rtx new_op0;
+ op0 = XEXP (x, 0);
+
+ /* There are some addresses that we cannot work on. */
+ if (!can_simplify_addr (op0))
+ return true;
+
+ op0 = new_op0 = targetm.delegitimize_address (op0);
+ valid_ops &= propagate_rtx_1 (&new_op0, old, new, true);
+
+ /* Dismiss transformation that we do not want to carry on. */
+ if (!valid_ops
+ || new_op0 == op0
+ || GET_MDE (new_op0) != GET_MDE (op0))
+ return true;
+
+ canonicalize_address (new_op0);
+
+ /* Copy propagations are always ok. check the costs. */
+ if (!(REG_P (old) && REG_P (new))
+ && !should_replace_address (op0, new_op0, GET_MDE (x)))
+ return true;
+
+ tem = replace_equiv_address_nv (x, new_op0);
+}
+
+ else if (code == LSUM)
+{
+ op0 = XEXP (x, 0);
+ op1 = XEXP (x, 1);
+
+ /* The only simplification we do attempts to remove references to op0
+ or make it constant -- in both cases, op0's invalidity will not
+ make the result invalid. */
+ propagate_rtx_1 (&op0, old, new, true);
+ valid_ops &= propagate_rtx_1 (&op1, old, new, can_appear);
+ if (op0 == XEXP (x, 0) && op1 == XEXP (x, 1))
+ return true;
+
+ /* (lo_sum (high x) x) -x */
+ if (GET_CDE (op0) == HIGH && rtx_equal_p (XEXP (op0, 0), op1))
+ tem = op1;
+ else
+ tem = gen_rtx_LSUM (mode, op0, op1);
+
+ /* P1 is likely not a legitimate address, otherwise there would have
+ been no LSUM. We want it to disappear if it is invalid, return
+ false in that case. */
+ return memory_address_p (mode, tem);
+}
+
+ else if (code == REG)
+{
+ if (rtx_equal_p (x, old))
+ {
+ *px = new;
+ return can_appear;
+ }
+}
+ break;
+
+ default:
+ break;
+ }
+
+ /* No change, no trouble. */
+ if (tem == NULL_RTX)
+ return true;
+
+ *px = tem;
+
+ /* The replacement we made so far is valid, if all of the recursive
+ replacements were valid, or we could simplify everything to
+ a constant. */
+ return valid_ops || can_appear || CNSTANT_P (tem);
+}
+
+/* Replace all occurrences of LD in X with NEW and try to simplify the
+ resulting expression (in mode MDE). Return a new expresion if it is
+ a constant, otherwise X.
+
+ Simplifications where occurrences of NEW collapse to a constant are always
+ accepted. All simplifications are accepted if NEW is a pseudo too.
+ , we accept simplifications that have a lower or equal cost. */
+
+static rtx
+propagate_rtx (rtx x, enum machine_mode mode, rtx old, rtx new)
+{
+ rtx tem;
+ bool collapsed;
+
+ if (REG_P (new) && REGN (new) < FIRST_PSEUDREGISTER)
+ return NULL_RTX;
+
+ new = copy_rtx (new);
+
+ tem = x;
+ collapsed = propagate_rtx_1 (&tem, old, new, REG_P (new) || CNSTANT_P (new));
+ if (tem == x || !collapsed)
+ return NULL_RTX;
+
+ /* gen_lowpart_common will not be able to process VIDmode entities other
+ than CNST_INTs. */
+ if (GET_MDE (tem) == VIDmode && GET_CDE (tem) != CNST_INT)
+ return NULL_RTX;
+
+ if (GET_MDE (tem) == VIDmode)
+ tem = rtl_hooks.gen_lowpart_no_emit (mode, tem);
+ else
+ gcc_assert (GET_MDE (tem) == mode);
+
+ return tem;
+}
+
+
+
+
+/* Return true if the register from reference REF is killed
+ between FRM to (but not including) T */
+
+static bool
+local_ref_killed_between_p (struct df_ref * ref, rtx from, rtx to)
+{
+ while (from != to)
+ {
+ struct df_ref * def = DF_INSN_DEFS (df, from);
+ while (def)
+{
+ if (DF_REF_REGN (ref) == DF_REF_REGN (def))
+ return true;
+ def = def->next_ref;
+}
+
+ from = NEXT_INSN (from);
+ }
+ return false;
+}
+
+
+/* Check if the given DEF is available in INSN. This would require full
+ computation of available expressions; we check only restricted conditions:
+ - if DEF is the sole definition of its register, go ahead;
+ - in the same basic block, we check for no definitions killing the
+ definition of DEF_INSN;
+ - if USE's basic block has DEF's basic block as the sole predecessor,
+ we check if the definition is killed after DEF_INSN or before
+ TARGET_INSN insn, in their respective basic blocks. */
+static bool
+use_killed_between (struct df_ref *use, rtx def_insn, rtx target_insn)
+{
+ basic_block def_bb, target_bb;
+ int regno;
+ struct df_ref * def;
+
+ /* Check if the reg in USE has only one definition. We already
+ know that this definition reaches use, or we wouldn't be here. */
+ regno = DF_REF_REGN (use);
+ def = DF_REG_DEF_GET (df, regno)->reg_chain;
+ if (def && (def->next_reg == NULL))
+ return false;
+
+ /* Check if we are in the same basic block. */
+ def_bb = BLCK_FR_INSN (def_insn);
+ target_bb = BLCK_FR_INSN (target_insn);
+ if (def_bb == target_bb)
+ return local_ref_killed_between_p (use, def_insn, target_insn);
+
+ /* Finally, if DEF_BB is the sole predecessor of TARGET_BB. */
+ if (single_pred_p (target_bb)
+ && single_pred (target_bb) == def_bb)
+ {
+ struct df_ref *x;
+
+ /* See if USE is killed between DEF_INSN and the last insn in the
+ basic block containing DEF_INSN. */
+ x = df_bb_regno_last_def_find (df, def_bb, regno);
+ if (x && DF_INSN_LUID (df, x->insn) >= DF_INSN_LUID (df, def_insn))
+return true;
+
+ /* See if USE is killed between TARGET_INSN and the first insn in the
+ basic block containing TARGET_INSN. */
+ x = df_bb_regno_first_def_find (df, target_bb, regno);
+ if (x && DF_INSN_LUID (df, x->insn) < DF_INSN_LUID (df, target_insn))
+return true;
+
+ return false;
+ }
+
+ /* assume the worst case. */
+ return true;
+}
+
+
+/* for_each_rtx traversal function that returns 1 if BDY points to
+ a non-constant mem. */
+
+static int
+varying_mem_p (rtx *body, void *data ATTRIBUTE_UNUSED)
+{
+ rtx x = *body;
+ return MEM_P (x) && !MEM_READNLY_P (x);
+}
+
+/* Check if all uses in DEF_INSN can be used in TARGET_INSN. This
+ would require full computation of available expressions;
+ we check only restricted conditions, see use_killed_between. */
+static bool
+all_uses_available_at (rtx def_insn, rtx target_insn)
+{
+ struct df_ref * use;
+ rtx def_set = single_set (def_insn);
+
+ gcc_assert (def_set);
+
+ /* If target_insn comes right after def_insn, which is very common
+ for addresses, we can use a quicker test. */
+ if (NEXT_INSN (def_insn) == target_insn
+ && REG_P (SET_DEST (def_set)))
+ {
+ rtx def_reg = SET_DEST (def_set);
+
+ /* If the insn uses the reg that it defines, the substitution is
+ invalid. */
+ for (use = DF_INSN_USES (df, def_insn); use; use = use->next_ref)
+ if (rtx_equal_p (use->reg, def_reg))
+ return false;
+ }
+ else
+ {
+ /* Look at all the uses of DEF_INSN, and see if they are not
+ killed between DEF_INSN and TARGET_INSN. */
+ for (use = DF_INSN_USES (df, def_insn); use; use = use->next_ref)
+if (use_killed_between (use, def_insn, target_insn))
+ return false;
+ }
+
+ /* We don't do any analysis of memories or aliasing. Reject any
+ instruction that involves references to non-constant memory. */
+ return !for_each_rtx (&SET_SRC (def_set), varying_mem_p, NULL);
+}
+
+
+
+/* Inside INSN, the expression rooted at *LC has been changed, moving some
+ uses from RIG_USES. Find those that are present, and create new items
+ in the data flow object of the pass. Mark any new uses as having the
+ given TYPE. */
+static void
+update_df (rtx insn, rtx *loc, struct df_ref *orig_uses, enum df_ref_type type,
+ int new_flags)
+{
+ struct df_ref *use;
+
+ /* Add a use for the registers that were propagated. */
+ for (use = orig_uses; use; use = use->next_ref)
+ {
+ struct df_ref *orig_use = use, *new_use;
+ rtx *new_loc = find_occurrence (loc, DF_REF_REG (orig_use));
+
+ if (!new_loc)
+continue;
+
+ /* Add a new insn use. Use the original type, because it says if the
+ use was within a MEM. */
+ new_use = df_ref_create (df, DF_REF_REG (orig_use), new_loc,
+ insn, BLCK_FR_INSN (insn),
+ type, DF_REF_FLAGS (orig_use) | new_flags);
+
+ /* Set up the use-def chain. */
+ df_chain_copy (df->problems_by_index[DF_CHAIN],
+ new_use, DF_REF_CHAIN (orig_use));
+ }
+}
+
+
+/* Try substituting NEW into LC, which originated from forward propagation
+ of USE's value from DEF_INSN. SET_REG_EQUAL says whether we are
+ substituting the whole SET_SRC, so we can set a REG_EQUAL note if the
+ new insn is not recognized. Return whether the substitution was
+ performed. */
+
+static bool
+try_fwprop_subst (struct df_ref *use, rtx *loc, rtx new, rtx def_insn, bool set_reg_equal)
+{
+ rtx insn = DF_REF_INSN (use);
+ enum df_ref_type type = DF_REF_TYPE (use);
+ int flags = DF_REF_FLAGS (use);
+
+ if (dump_file)
+ {
+ fprintf (dump_file, "\nIn insn %d, replacing\n ", INSN_UID (insn));
+ print_inline_rtx (dump_file, *loc, 2);
+ fprintf (dump_file, "\n with ");
+ print_inline_rtx (dump_file, new, 2);
+ fprintf (dump_file, "\n");
+ }
+
+ if (validate_change (insn, loc, new, false))
+ {
+ num_changes++;
+ if (dump_file)
+fprintf (dump_file, "Changed insn %d\n", INSN_UID (insn));
+
+ /* Unlink the use that we changed. */
+ df_ref_remove (df, use);
+ if (!CNSTANT_P (new))
+update_df (insn, loc, DF_INSN_USES (df, def_insn), type, flags);
+
+ return true;
+ }
+ else
+ {
+ if (dump_file)
+fprintf (dump_file, "Changes to insn %d not recognized\n",
+ INSN_UID (insn));
+
+ /* Can also record a simplified value in a REG_EQUAL note, making a
+ new one if one does not already exist. */
+ if (set_reg_equal)
+{
+ if (dump_file)
+ fprintf (dump_file, " Setting REG_EQUAL note\n");
+
+ REG_NTES (insn) = gen_rtx_EXPR_LIST (REG_EQUAL, copy_rtx (new),
+REG_NTES (insn));
+
+ if (!CNSTANT_P (new))
+ update_df (insn, loc, DF_INSN_USES (df, def_insn),
+ type, DF_REF_IN_NTE);
+}
+
+ return false;
+ }
+}
+
+
+/* If USE is a paradoxical subreg, see if it can be replaced by a pseudo. */
+
+static bool
+forward_propagate_subreg (struct df_ref *use, rtx def_insn, rtx def_set)
+{
+ rtx use_reg = DF_REF_REG (use);
+ rtx use_insn, src;
+
+ /* consider paradoxical subregs */
+ enum machine_mode use_mode = GET_MDE (use_reg);
+ if (GET_CDE (use_reg) != SUBREG
+ || !REG_P (SET_DEST (def_set))
+ || GET_MDE_SIZE (use_mode)
+ <= GET_MDE_SIZE (GET_MDE (SUBREG_REG (use_reg))))
+ return false;
+
+ /* If this is a paradoxical SUBREG, we have no idea what value the
+ extra bits would have. However, if the operand is equivalent to
+ a SUBREG whose operand is the same as our mode, and all the modes
+ are within a word, we can just use the inner operand because
+ these SUBREGs just say how to treat the register. */
+ use_insn = DF_REF_INSN (use);
+ src = SET_SRC (def_set);
+ if (GET_CDE (src) == SUBREG
+ && REG_P (SUBREG_REG (src))
+ && GET_MDE (SUBREG_REG (src)) == use_mode
+ && subreg_lowpart_p (src)
+ && all_uses_available_at (def_insn, use_insn))
+ return try_fwprop_subst (use, DF_REF_LC (use), SUBREG_REG (src),
+ def_insn, false);
+ else
+ return false;
+}
+
+/* Try to replace USE with SRC (defined in DEF_INSN) and simplify the
+ result. */
+
+static bool
+forward_propagate_and_simplify (struct df_ref *use, rtx def_insn, rtx def_set)
+{
+ rtx use_insn = DF_REF_INSN (use);
+ rtx use_set = single_set (use_insn);
+ rtx src, reg, new, *loc;
+ bool set_reg_equal;
+ enum machine_mode mode;
+
+ if (!use_set)
+ return false;
+
+ /* Do not propagate into PC, CC0, etc. */
+ if (GET_MDE (SET_DEST (use_set)) == VIDmode)
+ return false;
+
+ /* If def and use are subreg, check if they match. */
+ reg = DF_REF_REG (use);
+ if (GET_CDE (reg) == SUBREG
+ && GET_CDE (SET_DEST (def_set)) == SUBREG
+ && (SUBREG_BYTE (SET_DEST (def_set)) != SUBREG_BYTE (reg)
+ || GET_MDE (SET_DEST (def_set)) != GET_MDE (reg)))
+ return false;
+
+ /* Check if the def had a subreg, but the use has the whole reg. */
+ if (REG_P (reg) && GET_CDE (SET_DEST (def_set)) == SUBREG)
+ return false;
+
+ /* Check if the use has a subreg, but the def had the whole reg. Unlike the
+ previous case, the optimization is possible and often useful indeed. */
+ if (GET_CDE (reg) == SUBREG && REG_P (SET_DEST (def_set)))
+ reg = SUBREG_REG (reg);
+
+ /* Check if the substitution is valid (last, because it's the most
+ expensive check!). */
+ src = SET_SRC (def_set);
+ if (!CNSTANT_P (src) && !all_uses_available_at (def_insn, use_insn))
+ return false;
+
+ /* Check if the def is loading something from the constant pool; in this
+ case we would undo optimization such as compress_float_constant.
+ Still, we can set a REG_EQUAL note. */
+ if (MEM_P (src) && MEM_READNLY_P (src))
+ {
+ rtx x = avoid_constant_pool_reference (src);
+ if (x != src)
+{
+ rtx note = find_reg_note (use_insn, REG_EQUAL, NULL_RTX);
+ rtx old = note ? XEXP (note, 0) : SET_SRC (use_set);
+ rtx new = simplify_replace_rtx (old, src, x);
+ if (old != new)
+ set_unique_reg_note (use_insn, REG_EQUAL, copy_rtx (new));
+}
+ return false;
+ }
+
+ /* Else try simplifying. */
+
+ if (DF_REF_TYPE (use) == DF_REF_REG_MEM_STRE)
+ {
+ loc = &SET_DEST (use_set);
+ set_reg_equal = false;
+ }
+ else
+ {
+ rtx note = find_reg_note (use_insn, REG_EQUAL, NULL_RTX);
+ if (DF_REF_FLAGS (use) & DF_REF_IN_NTE)
+loc = &XEXP (note, 0);
+ else
+loc = &SET_SRC (use_set);
+
+ /* Do not replace an existing REG_EQUAL note if the insn is not
+ recognized. Either we're already replacing in the note, or
+ we'll separately try plugging the definition in the note and
+ simplifying. */
+ set_reg_equal = (note == NULL_RTX);
+ }
+
+ if (GET_MDE (*loc) == VIDmode)
+ mode = GET_MDE (SET_DEST (use_set));
+ else
+ mode = GET_MDE (*loc);
+
+ new = propagate_rtx (*loc, mode, reg, src);
+
+ if (!new)
+ return false;
+
+ return try_fwprop_subst (use, loc, new, def_insn, set_reg_equal);
+}
+
+
+/* Given a use USE of an insn, if it has a single reaching
+ definition, try to forward propagate it into that insn. */
+
+static void
+forward_propagate_into (struct df_ref *use)
+{
+ struct df_link *defs;
+ struct df_ref *def;
+ rtx def_insn, def_set, use_insn;
+ rtx parent;
+
+ if (DF_REF_FLAGS (use) & DF_REF_READ_WRITE)
+ return;
+
+ /* consider uses that have a single definition. */
+ defs = DF_REF_CHAIN (use);
+ if (!defs || defs->next)
+ return;
+
+ def = defs->ref;
+ if (DF_REF_FLAGS (def) & DF_REF_READ_WRITE)
+ return;
+
+ /* Do not propagate loop invariant definitions inside the loop if
+ we are going to unroll. */
+ if (loops.num 0
+ && DF_REF_BB (def)->loop_father != DF_REF_BB (use)->loop_father)
+ return;
+
+ /* Check if the use is still present in the insn! */
+ use_insn = DF_REF_INSN (use);
+ if (DF_REF_FLAGS (use) & DF_REF_IN_NTE)
+ parent = find_reg_note (use_insn, REG_EQUAL, NULL_RTX);
+ else
+ parent = PATTERN (use_insn);
+
+ if (!loc_mentioned_in_p (DF_REF_LC (use), parent))
+ return;
+
+ def_insn = DF_REF_INSN (def);
+ def_set = single_set (def_insn);
+ if (!def_set)
+ return;
+
+ /* try one kind of propagation. If two are possible, we'll
+ do it on the following iterations. */
+ if (!forward_propagate_and_simplify (use, def_insn, def_set))
+ forward_propagate_subreg (use, def_insn, def_set);
+}
+
+
+static void
+fwprop_init (void)
+{
+ num_changes = 0;
+
+ /* We do not always want to propagate into loops, so we have to find
+ loops and be careful about them. But we have to call flow_loops_find
+ before df_analyze, because flow_loops_find may introduce new jump
+ insns (sadly) if we are not working in cfglayout mode. */
+ if (flag_rerun_cse_after_loop && (flag_unroll_loops || flag_peel_loops))
+ {
+ calculate_dominance_info (CDI_DMINATRS);
+ flow_loops_find (&loops);
+ }
+
+ /* Now set up the dataflow problem (we only want use-def chains) and
+ put the dataflow solver to work. */
+ df = df_init (DF_SUBREGS | DF_EQUIV_NTES);
+ df_chain_add_problem (df, DF_UD_CHAIN);
+ df_analyze (df);
+ df_dump (df, dump_file);
+}
+
+static void
+fwprop_done (void)
+{
+ df_finish (df);
+ cleanup_cfg (0);
+ delete_trivially_dead_insns (get_insns (), max_reg_num ());
+
+ if (dump_file)
+ fprintf (dump_file,
+ "\nNumber of successful forward propagations: %d\n\n",
+ num_changes);
+
+ if (flag_rerun_cse_after_loop && (flag_unroll_loops || flag_peel_loops))
+ {
+ flow_loops_free (&loops);
+ free_dominance_info (CDI_DMINATRS);
+ loops.num = 0;
+ }
+
+}
+
+
+
+/* Main entry point. */
+
+static bool
+gate_fwprop (void)
+{
+ return optimize 0 && flag_forward_propagate;
+}
+
+static unsigned int
+fwprop (void)
+{
+ unsigned i;
+
+ fwprop_init ();
+
+ /* Go through all the uses. update_df will create new ones at the
+ end, and we'll go through them as well.
+
+ Do not forward propagate addresses into loops until after unrolling.
+ CSE did so because it was able to fix its own mess, but we are not. */
+
+ df_reorganize_refs (&df->use_info);
+ for (i = 0; i < DF_USES_SIZE (df); i++)
+ {
+ struct df_ref *use = DF_USES_GET (df, i);
+ if (use)
+if (loops.num == 0
+ || DF_REF_TYPE (use) == DF_REF_REG_USE
+ || DF_REF_BB (use)->loop_father == NULL)
+ forward_propagate_into (use);
+ }
+
+ fwprop_done ();
+
+ return 0;
+}
+
+struct tree_opt_pass pass_rtl_fwprop =
+{
+ "fwprop1", /* name */
+ gate_fwprop,/* gate */
+ fwprop,/* execute */
+ NULL, /* sub */
+ NULL, /* next */
+ 0, /* static_pass_number */
+ TV_FWPRP, /* tv_id */
+ 0, /* properties_required */
+ 0, /* properties_provided */
+ 0, /* properties_destroyed */
+ 0, /* todo_flags_start */
+ TD, /* todo_flags_finish */
+ 0 /* letter */
+};
+
+static bool
+gate_fwprop_addr (void)
+{
+ return optimize 0 && flag_forward_propagate && flag_rerun_cse_after_loop
+ && (flag_unroll_loops || flag_peel_loops);
+}
+
+static unsigned int
+fwprop_addr (void)
+{
+ unsigned i;
+ fwprop_init ();
+
+ /* Go through all the uses. update_df will create new ones at the
+ end, and we'll go through them as well. */
+ df_reorganize_refs (&df->use_info);
+ for (i = 0; i < DF_USES_SIZE (df); i++)
+ {
+ struct df_ref *use = DF_USES_GET (df, i);
+ if (use)
+if (DF_REF_TYPE (use) != DF_REF_REG_USE
+ && DF_REF_BB (use)->loop_father != NULL)
+ forward_propagate_into (use);
+ }
+
+ fwprop_done ();
+
+ return 0;
+}
+
+struct tree_opt_pass pass_rtl_fwprop_addr =
+{
+ "fwprop2", /* name */
+ gate_fwprop_addr,/* gate */
+ fwprop_addr,/* execute */
+ NULL, /* sub */
+ NULL, /* next */
+ 0, /* static_pass_number */
+ TV_FWPRP, /* tv_id */
+ 0, /* properties_required */
+ 0, /* properties_provided */
+ 0, /* properties_destroyed */
+ 0, /* todo_flags_start */
+ TD, /* todo_flags_finish */
+ 0 /* letter */
+};

Sunday 14 May 2006 02:13, Steven Bosscher wrote:
So this is the first part of the changes. The second bits that clean
up cse.c are unchanged. I have only tested this with a C bootstrap on
x86-linux, but I'll give it a full run tomorrow and if it passes, I'll
post it as an RFT.

So this is the fully tested version.
Bootstrapped and tested on x86_64-suse-linux-gnu, multilib, all languages
except treelang.

David, Richard, could you give these patches a re-try on SPEC please?

Gr.
Steven

Sunday 14 May 2006 18:37, Steven Bosscher wrote:
So this is the fully tested version.
Bootstrapped and tested on x86_64-suse-linux-gnu, multilib, all languages
except treelang.

I've also tested the patches on sh-sim and mips-sim (C only, no
regressions), and I've built a compiler for with (like
in e.g. ).

I don't know which target board to use to test for avr. mstein, you
tested on atmega128-sim but I don't have such a board. So what board
should I use for testing on avr?

Gr.
Steven

Steven Bosscher wrote:

I don't know which target board to use to test for avr. mstein, you
tested on atmega128-sim but I don't have such a board. So what board
should I use for testing on avr?

I use the free simulator "simulavr", which is not in the fsf tree.
I patched trunk (r113748) with fwprop_[12].diff. You should get the
results on tuesday morning.

Mike

Sun, 14 May 2006, Steven Bosscher wrote:

Sunday 14 May 2006 02:13, Steven Bosscher wrote:
So this is the first part of the changes. The second bits that clean
up cse.c are unchanged. I have only tested this with a C bootstrap on
x86-linux, but I'll give it a full run tomorrow and if it passes, I'll
post it as an RFT.

So this is the fully tested version.
Bootstrapped and tested on x86_64-suse-linux-gnu, multilib, all languages
except treelang.

David, Richard, could you give these patches a re-try on SPEC please?

I have done SPEC2k testing on x86_64 with and on i686 (x86_64 -m32).
Full results attached, the summaries look like

x86_64:

164.gzip 1400 165 846 * 1400 159 881
*
175.vpr 1400 157 893 * 1400 157 891
*
176.gcc X
X
181.mcf 1800 321 561 * 1800 323 557
*
186.crafty 1000 64.5 1551 * 1000 66.2 1511
*
197.parser 1800 259 696 * 1800 258 698
*
252.eon 1300 89.1 1458 * 1300 90.1 1442
*
253.perlbmk 1800 160 1122 * 1800 158 1140
*
254.gap 1100 117 939 * 1100 115 957
*
255.vortex X
X
256.bzip2 1500 163 920 * 1500 163 918
*
300.twolf 3000 295 1019 * 3000 294 1021
*

168.wupwise 1600 150 1064 * 1600 151
1059*
171.swim 3100 382 811 * 3100 382
811*
172.mgrid 1800 246 732 * 1800 248
725*
173.applu 2100 265 793 * 2100 264
795*
177.mesa 1400 117 1193 * 1400 119
1179*
178.galgel 2900 194
1493*
179.art 2600 246 1058 * 2600 251
1036*
183.equake 1300 123 1056 * 1300 123
1055*
187.facerec 1900 227 837 * 1900 227
835*
188.ammp 2200 228 965 * 2200 228
963*
189.lucas 2000 183 1093 * 2000 183
1095*
191.fma3d 2100 258 813 * 2100 257
816*
200.sixtrack 1100 258 426 * 1100 258
426*
301.apsi 2600 279 932 * 2600 279
931*

i686:

164.gzip 1400 179 782 * 1400 181 772
*
175.vpr 1400 170 824 * 1400 170 822
*
176.gcc X
X
181.mcf 1800 286 630 * 1800 286 630
*
186.crafty 1000 97.0 1031 * 1000 97.2 1029
*
197.parser 1800 253 713 * 1800 254 707
*
252.eon X
X
253.perlbmk 1800 164 1097 * 1800 165 1091
*
254.gap 1100 129 853 * 1100 130 848
*
255.vortex 1900 162 1175 * 1900 159 1195
*
256.bzip2 1500 203 738 * 1500 199 753
*
300.twolf 3000 269 1114 * 3000 268 1118
*

168.wupwise 1600 179 892* 1600 177
902*
171.swim 3100 485 639* 3100 485
639*
172.mgrid 1800 369 488* 1800 368
489*
173.applu 2100 355 592* 2100 355
592*
177.mesa 1400 156 900* 1400 162
866*
178.galgel 2900 220 1315* 2900 224
1296*
179.art 2600 641 405* 2600 627
414*
183.equake 1300 155 836* 1300 154
846*
187.facerec 1900 249 762* 1900 250
761*
188.ammp 2200 282 779* 2200 282
779*
189.lucas 2000 279 717* 2000 279
717*
191.fma3d 2100 295 713* 2100 294
715*
200.sixtrack 1100 219 502* 1100 218
504*
301.apsi 2600 342 760* 2600 341
762*

I also patched in for the May14 run of
http://www.suse.de/~rguenther/c++bench and it didn't show any effects
on compile-time, memory-usage or performance.

Richard.

5/15/06, Richard Guenther <rguenther (AT) suse (DOT) dewrote:
I also patched in for the May14 run of
http://www.suse.de/~rguenther/c++bench and it didn't show any effects
on compile-time, memory-usage or performance.

What happened to Wave (http://www.suse.de/~rguenther/c++bench/boost/)?
Is that due to fwprop or did something else change?

Gr.
Steven

5/15/06, Steven Bosscher <stevenb.gcc (AT) gmail (DOT) comwrote:
5/15/06, Richard Guenther <rguenther (AT) suse (DOT) dewrote:
I also patched in for the May14 run of
http://www.suse.de/~rguenther/c++bench and it didn't show any effects
on compile-time, memory-usage or performance.

What happened to Wave (http://www.suse.de/~rguenther/c++bench/boost/)?
Is that due to fwprop or did something else change?

No, I fixed wave - it's only last vs. previous-of-last datapoint that
is interesting.

Richard.

5/15/06, Serge Belyshev <belyshev (AT) depni (DOT) sinp.msu.ruwrote:
Steven Bosscher <stevenb.gcc (AT) gmail (DOT) comwrites:

Sunday 14 May 2006 02:13, Steven Bosscher wrote:

David, Richard, could you give these patches a re-try on SPEC please?

176.gcc fails to compile on an amd64 host with -funroll-loops. (infinite loop after fwprop)
Here is partially reduced testcase:

First of all, thanks a lot for that test case, and for trying these patches!

The problem you're seeing is caused by the fact that fwprop doesn't
update dominator info when it modifies the CFG. Therefore it should
free dominance info before doing a cfgcleanup. The patch below fixes
that problem.

Gr.
Steven

fwprop.c.old 2006-05-15 22:20:11.000000000 +0200
fwprop.c 2006-05-15 22:20:07.000000000 +0200
@@ -853,25 +853,25 @@ fwprop_init (void)

static void
fwprop_done (void)
{
df_finish (df);
- cleanup_cfg (0);
- delete_trivially_dead_insns (get_insns (), max_reg_num ());
-
- if (dump_file)
- fprintf (dump_file,
- "\nNumber of successful forward propagations: %d\n\n",
- num_changes);

if (flag_rerun_cse_after_loop && (flag_unroll_loops || flag_peel_loops))
{
flow_loops_free (&loops);
free_dominance_info (CDI_DMINATRS);
loops.num = 0;
}

+ cleanup_cfg (0);
+ delete_trivially_dead_insns (get_insns (), max_reg_num ());
+
+ if (dump_file)
+ fprintf (dump_file,
+ "\nNumber of successful forward propagations: %d\n\n",
+ num_changes);
}

/* Main entry point. */

I have run SPEC CPU2000 on a PowerMac G5 with and without the
fwprop patch.

BASE is -mcpu=G5 -mdynamic-no-pic -malign-natural
PEAK is -mcpu=G5 -ffast-math -funroll-loops -fpeel-loops -mdynamic-no-pic -malign-natural

The percent difference in the SPEC score is:
BASEPEAK
gzip:0%1%
vpr:0%0%
gcc:0%0%
mcf:0%0%
crafty:0%0%
parser:0%0%
eon:0%3%
perlbmk:-1%-1%
gap:-9%-3%
vortex:0%0%
bzip2:0%0%
twolf:0%0%
wupwise:0%0%
swim:0%0%
mgrid:0%0%
applu:-11%
mesa:-2%-2%
galgel:0%0%
art:0%0%
equake:0%0%
facerec:0%0%
ammp:0%0%
lucas:0%0%
fma3d:0%0%
sixtrack:0%0%
apsi:0%0%

than gap, the effect of the patch is in the noise. The
decrease in performance can be addressed after the patch is merged.

I also am rerunning the SPEC tests without the 32/64 mixed mode.

David

David Edelsohn wrote:
I have run SPEC CPU2000 on a PowerMac G5 with and without the
fwprop patch.

Thank you for doing this.

The percent difference in the SPEC score is:

These are individual scores, what about the total?

than gap, the effect of the patch is in the noise. The
decrease in performance can be addressed after the patch is merged.

Still, it would be nice to understand it before merging the patch. A 9%
regression, while only for one program, is still significant. Is it
consistent across multiple runs? Can you do something to narrow down
the problem to one object file or one function?

Bernd

Bernd Schmidt writes:

>The percent difference in the SPEC score is:
BerndThese are individual scores, what about the total?
The change of the total, geometric mean score is in the noise.
David

Sunday 14 May 2006 18:37, Steven Bosscher wrote:
Sunday 14 May 2006 02:13, Steven Bosscher wrote:
So this is the first part of the changes. The second bits that clean
up cse.c are unchanged. I have only tested this with a C bootstrap on
x86-linux, but I'll give it a full run tomorrow and if it passes, I'll
post it as an RFT.

So this is the fully tested version.
Bootstrapped and tested on x86_64-suse-linux-gnu, multilib, all languages
except treelang.

After Serge's testing uncovered a bug, I had to change the fwprop-1
patch to free dominance info before calling cfgcleanup. I also worked
with Zdenek to make find_occurrence use for_each_rtx and while doing
that I moved it from rtlanal.c to fwprop.c (it's the only user anyway).

Mike Stein was so kind to test C on a large number of targets:
v850-unknown-elf
sh-unknown-elf
mn10300-unknown-elf
mips-unknown-elf
m68hc11-unknown-none
m32r-unknown-elf
h8300-unknown-elf
bfin-unknown-elf
avr-unknown-none
arm-unknown-elf

In this testing, a number of new failures appeared:

# of new failures:
v850-unknown-elf:4
sh-unknown-elf:0
mn10300-unknown-elf:6
mips-unknown-elf:4
m68hc11-unknown-none:5
m32r-unknown-elf:8
h8300-unknown-elf:28
bfin-unknown-elf:0
avr-unknown-none:6
arm-unknown-elf:4

# of failures that disappeared
h8300-unknown-elf:25
m68hc11-unknown-none:5

I looked at the new failures to see what could explain them:
* there are 65 new failures in total for these 10 targets.
* 20 are at or higher,
so they are probably all the same problem (amr:4, m32r:4, mips:4,
mn10300:4, v850:4).
* 5 are at and higher
(m32r:4 execution, 1*m68hc11:1 execution, but it didn't compile before).
* 4 are (m68hc11:2, mn10300:2).
* 12 of them are with -funroll-{all-,}loops and not one of the above
test cases, which means they are probably the same as Serge's infinite
loop (h8300:8, m32r:2, m68hc11:2)

Testing (multilib) on x86-64, all languages, still doesn't show any
regressions. I'm obviously going to investigate the new failures the
best I can, and resubmit the patches when I'm happy with the results.
In the mean time, these are the latest patches, in case people want
to help.

Gr.
Steven

Still, it would be nice to understand it before merging the patch. A 9%
regression, while only for one program, is still significant. Is it
consistent across multiple runs? Can you do something to narrow down
the problem to one object file or one function?

This would help definitely because I'm not seeing that on a G4.

Paolo

Friday 19 May 2006 00:36, Steven Bosscher wrote:
* 20 are at or higher,
so they are probably all the same problem (amr:4, m32r:4, mips:4,
mn10300:4, v850:4).

This failure turned out to be a problem where we have a DEF that
reaches some USE in the same basic block that the DEF is in, but
the DEF does not dominate the USE. Think e.g. using a register
that is initialized in a loop, but it is used in the same loop
before it is initialized.

The fix has two parts:
1) Don't look at notes in local_ref_killed_between_p. This was
causing the ICE for this test case because from was after to,
so we'd run into the basic block note of the next basic block.
2) Don't call local_ref_killed_between_p from use_killed_between
when def_insn comes after target_insn.

This patch is incremental wrt. the patches I posted last night.
They still need to go through the full cycle of testing that I
can do myself.

Gr.
Steven

fwprop.c.jj2006-05-18 20:44:07.000000000 +0200
fwprop.c2006-05-20 00:26:38.000000000 +0200
@@ -433,21 +433,24 @@ propagate_rtx (rtx x, enum machine_mode
between FRM to (but not including) T */

static bool
local_ref_killed_between_p (struct df_ref * ref, rtx from, rtx to)
{
- while (from != to)
+ rtx insn;
+
+ for (insn = from; insn != to; insn = NEXT_INSN (insn))
{
- struct df_ref * def = DF_INSN_DEFS (df, from);
+ if (!INSN_P (insn))
+continue;
+
+ struct df_ref * def = DF_INSN_DEFS (df, insn);
while (def)
{
if (DF_REF_REGN (ref) == DF_REF_REGN (def))
return true;
def = def->next_ref;
}
-
- from = NEXT_INSN (from);
}
return false;
}

@@ -475,11 +478,22 @@ use_killed_between (struct df_ref *use,

/* Check if we are in the same basic block. */
def_bb = BLCK_FR_INSN (def_insn);
target_bb = BLCK_FR_INSN (target_insn);
if (def_bb == target_bb)
- return local_ref_killed_between_p (use, def_insn, target_insn);
+ {
+ /* In some obscure situations we can have a def reaching a use
+ that is _before_ the def. In other words the def does not
+ dominate the use even though the use and def are in the same
+ basic block. This can happen when a register may be used
+ uninitialized in a loop. In such cases, we must assume that
+ DEF is not available. */
+ if (DF_INSN_LUID (df, def_insn) >= DF_INSN_LUID (df, target_insn))
+return true;
+
+ return local_ref_killed_between_p (use, def_insn, target_insn);
+ }

/* Finally, if DEF_BB is the sole predecessor of TARGET_BB. */
if (single_pred_p (target_bb)
&& single_pred (target_bb) == def_bb)
{

I bootstrapped and regression tested the patch on
powerpc-ibm-aix5.3.0.0. There were no changes in regressions.

I re-ran SPEC in 32-bit only mode. The differences are:

BASEPEAK
gzip:10
vpr:00
gcc:00
mcf:00
crafty:-1-1
parser:00
eon:81
perlbmk:0-1
gap:-28
vortex:00
bzip2:0-1
twolf:00
wupwise:00
swim:00
mgrid:00
applu:00
mesa:-10
galgel:00
art:01
equake:00
facerec:00
ammp:00
lucas:00
fma3d:00
sixtrack:00
apsi:0-2

Note that gap now jumps up 8%. internal tester shows quite
wide variance for gap -- up to 15% day to day. I think the patch has a
generally neutral effect.

David

Steven Bosscher <stevenb.gcc (AT) gmail (DOT) comwrote:
In the mean time, these are the latest patches, in case people want
to help.

I've regtested it for x86 cross sh4-unknown-linux-gnu and
sh64-unknown-linux-gnu. There are no new failures on both
targets and it cures some FAILs on sh4-unknown-linux-gnu:

tests that failed, that have disappeared: (Eeek!)

PR26858 execution - source compiled test
execution test
-fomit-frame-pointer execution test
-fomit-frame-pointer -funroll-all-loops -finline-functions execution test
-fomit-frame-pointer -funroll-loops execution test
-g execution test

Regards,
kaz