| Message ID | 4EB275D8.5070209@gjlay.de |
|---|---|
| State | New |
| Headers | show |
> PR rtl-optimization/50448 > * cprop.c (try_replace_reg): Try to simplify SET_SRC given the > substitution. The whole patch is about SET_DEST though, so I'm a little confused. And the head comment of try_replace_reg reads: /* Try to replace all non-SET_DEST occurrences of FROM in INSN with TO. Returns nonzero is successful. */ so I'm not sure this is the right place to fiddle with SET_DEST.
On 11/03/2011 10:21 PM, Eric Botcazou wrote: >> PR rtl-optimization/50448 >> > * cprop.c (try_replace_reg): Try to simplify SET_SRC given the >> > substitution. > The whole patch is about SET_DEST though, so I'm a little confused. Yes, the changelog is wrong indeed. Registers in a SET_DEST memory are uses, so they are like SET_SRC in this context which is why I think the patch does belong in try_replace_reg. Georg, what do you think of a changelog like this: Also try to replace uses of FROM that appear in SET_DEST. > And the > head comment of try_replace_reg reads: > > /* Try to replace all non-SET_DEST occurrences of FROM in INSN with TO. > Returns nonzero is successful. */ I agree; like above, the patch should also change the head comment like this: /* Try to replace all uses of FROM in INSN with TO. Returns nonzero is successful. */ Paolo
> Yes, the changelog is wrong indeed. Registers in a SET_DEST memory are > uses, so they are like SET_SRC in this context which is why I think the > patch does belong in try_replace_reg. Georg, what do you think of a > changelog like this: > > Also try to replace uses of FROM that appear in SET_DEST. OK, this makes sense. validate_replace_src_group actually does this too. > I agree; like above, the patch should also change the head comment like > this: > > /* Try to replace all uses of FROM in INSN with TO. Returns > nonzero is successful. */ No 's' in "Returns". Note that the comment is also off: + /* If above failed and this is a single set, try to simplify the source of + the set given our substitution. We could perhaps try this for multiple + SETs, but it probably won't buy us anything. */ + rtx addr = simplify_replace_rtx (SET_DEST (set), from, to); What does "If above failed" refer to? Again "source" instead of "destination".
On 11/04/2011 09:50 AM, Eric Botcazou wrote: > + /* If above failed and this is a single set, try to simplify the source of > + the set given our substitution. We could perhaps try this for multiple > + SETs, but it probably won't buy us anything. */ > + rtx addr = simplify_replace_rtx (SET_DEST (set), from, to); > > What does "If above failed" refer to? Again "source" instead of "destination". What about /* Registers can also appear as uses in SET_DEST if it is a MEM. We could perhaps try this for multiple SETs, but it probably won't buy us anything. */ ? Georg, can you put it all together into a v2? Paolo
> What about > > /* Registers can also appear as uses in SET_DEST if it is a MEM. We > could perhaps try this for multiple SETs, but it probably won't > buy us anything. */ > > ? Fine with me, thanks.
Index: cprop.c =================================================================== --- cprop.c (revision 180654) +++ cprop.c (working copy) @@ -764,6 +764,18 @@ try_replace_reg (rtx from, rtx to, rtx i note = set_unique_reg_note (insn, REG_EQUAL, copy_rtx (src)); } + if (set && MEM_P (SET_DEST (set)) && reg_mentioned_p (from, SET_DEST (set))) + { + /* If above failed and this is a single set, try to simplify the source of + the set given our substitution. We could perhaps try this for multiple + SETs, but it probably won't buy us anything. */ + rtx addr = simplify_replace_rtx (SET_DEST (set), from, to); + + if (!rtx_equal_p (addr, SET_DEST (set)) + && validate_change (insn, &SET_DEST (set), addr, 0)) + success = 1; + } + /* REG_EQUAL may get simplified into register. We don't allow that. Remove that note. This code ought not to happen, because previous code ought to synthesize