Question about merging two instructions.

Question about merging two instructions.

[Leehod Baruch]

Hello,

I'm working on a the sign extension elimination pass.
(see http://gcc.gnu.org/ml/gcc-patches/2005-08/msg01087.html for details)
I would like to merge these two instructions:

(insn 1 0 2 0 (set (reg/v:Xmode r)
        (sign_extend:Xmode (op:Ymode (...))))
(insn 2 1 3 0 (set (lhs) (rhs)))

where:
1. Xmode > Ymode
2. rhs and/or lhs may contain: (subreg:Ymode (reg/v:Xmode r) lowpart)

The extension may be redundant here.
I would like to merge these instructions and eliminate the extension, 
if possible.

I tried to use simplify_replace_rtx to replace any use of (reg r) with 
the right-hand-side of the extension and simplify the result.
But, it didn't work till I added this change:

Index: simplify-rtx.c
===================================================================
RCS file: /cvsroot/gcc/gcc/gcc/simplify-rtx.c,v
retrieving revision 1.243
diff -c -3 -p -r1.243 simplify-rtx.c
*** simplify-rtx.c      16 Aug 2005 02:01:27 -0000      1.243
--- simplify-rtx.c      21 Aug 2005 12:07:57 -0000
*************** simplify_replace_rtx (rtx x, rtx old_rtx
*** 319,325 ****
        return simplify_gen_ternary (code, mode, op_mode, op0, op1, op2);

      case RTX_EXTRA:
-       /* The only case we try to handle is a SUBREG.  */
        if (code == SUBREG)
        {
          op0 = simplify_replace_rtx (SUBREG_REG (x), old_rtx, new_rtx);
--- 319,324 ----
*************** simplify_replace_rtx (rtx x, rtx old_rtx
*** 329,334 ****
--- 328,341 ----
                                     GET_MODE (SUBREG_REG (x)),
                                     SUBREG_BYTE (x));
          return op0 ? op0 : x;
+       }
+       if (code == SET)
+       {
+         op0 = simplify_replace_rtx (XEXP (x, 0), old_rtx, new_rtx);
+         op1 = simplify_replace_rtx (XEXP (x, 1), old_rtx, new_rtx);
+         if ((op0 == XEXP (x, 0)) && (op1 == XEXP (x, 1)))
+           return x;
+         return gen_rtx_SET (VOIDmode, op0, op1);
        }
        break;

This way, when the replacement succeeds and the result instruction 
is recognizable, I can eliminate the extension.

Does it seem like a good change?
Does anyone have a better solution?


Thanks,
Leehod.

Re: Question about merging two instructions.

[Roger Sayle]

On Sun, 21 Aug 2005, Leehod Baruch wrote:
> *** 329,334 ****
> --- 328,341 ----
>                                      GET_MODE (SUBREG_REG (x)),
>                                      SUBREG_BYTE (x));
>           return op0 ? op0 : x;
> +       }
> +       if (code == SET)
> +       {
> +         op0 = simplify_replace_rtx (XEXP (x, 0), old_rtx, new_rtx);
> +         op1 = simplify_replace_rtx (XEXP (x, 1), old_rtx, new_rtx);
> +         if ((op0 == XEXP (x, 0)) && (op1 == XEXP (x, 1)))
> +           return x;
> +         return gen_rtx_SET (VOIDmode, op0, op1);
>         }
>         break;
>
> This way, when the replacement succeeds and the result instruction
> is recognizable, I can eliminate the extension.
>
> Does it seem like a good change?
> Does anyone have a better solution?

This approach seems reasonable.  The current structure of the code
in simplify_replace_rtx is intended to handle RTL expressions rather
than patterns, so normally it would be passed just SET_SRC (pat),
instead of the whole set.

The reason why no-one has wanted/required simplify_replace_rtx to
work on SETs is that all current passes tend to be cautious about
changes to the LHS of an assignment.  Performing substitution and
simplifications in the RHS/SET_SRC always produces a semantically
equivalent instruction, but blindly replacing subexpressions in
the LHS/SET_DEST can significantly change its meaning.

Hence, when combine/GCSE/CSE/reload and other RTL optimization
passes modify the target/destination of an assignment, they only
perform the transformations they expect or can guarantee are safe
(building the SET themselves), but are happy to let simplify_replace_rtx
do it's thing on the source expression.


To summarise, the change above is not unreasonable and I'd be
happy to allow this change to simplify-rtx.c, but I'd be more
cautious about where and why it was used.  For example, if you're
sure nothing bad can happen in the LHS, it might be reasonable
to instead place this code in a simplify_replace_set() function.
As the SET (and perhaps support for PARALLEL) should only ever
occur at the top-level, which can then call the recursive
simplify_replace_rtx.

I hope this helps.

Roger
--

Re: Question about merging two instructions.

[Leehod Baruch]

>>(insn 1 0 2 0 (set (reg/v:Xmode r)
>>        (sign_extend:Xmode (op:Ymode (...))))
>>(insn 2 1 3 0 (set (lhs) (rhs)))

> To summarise, the change above is not unreasonable and I'd be
> happy to allow this change to simplify-rtx.c, but I'd be more
> cautious about where and why it was used.  For example, if you're
> sure nothing bad can happen in the LHS, it might be reasonable
> to instead place this code in a simplify_replace_set() function.

I want to replace every use of (reg r) with the extension.  There might 
be such use on the LHS, this is why the LHS might change.

1. Can you please give me an example of something bad that can happen to 
the LHS.  Maybe I'm missing something here.

2. After calling simplify_replace_rtx I try to recognize the instruction.
Is this been cautious or is it unnecessary?

3. Isn't it reasonable to expect that every instance on old_rtx will be 
replaced by new_rtx even if it can't be simplified?
This is what I understand from the function's documentation.
But actually every expressions that can't be simplified is not replaced.


Thanks,
Leehod.

Re: Question about merging two instructions.

[Paolo Bonzini]

> 1. Can you please give me an example of something bad that can happen to 
> the LHS.  Maybe I'm missing something here.

In this case nothing, but if NEW were a subreg, it can change a lot.

> 3. Isn't it reasonable to expect that every instance on old_rtx will be 
> replaced by new_rtx even if it can't be simplified?
> This is what I understand from the function's documentation.
> But actually every expressions that can't be simplified is not replaced.

SET is not an expression, so it is not handled by simplify_replace_rtx.

I agree that it is confusing, that

   x = simplify_replace_rtx (a, b, c);

is different from

   x = simplify_rtx (replace_rtx (a, b, c));

So maybe all you need is changing the final "return x" to "return 
replace_rtx (x, old, new);".  I reckon this change would be much cleaner 
than handling SET, but I would very much want to see what a GWP person 
thinks about this.

Paolo

Re: Question about merging two instructions.

[Leehod Baruch]

> > 1. Can you please give me an example of something bad that can happen 
to 
> > the LHS.  Maybe I'm missing something here.
> 
> In this case nothing, but if NEW were a subreg, it can change a lot.

Why? 

Do I always need to recognize the result?
If the answer is yes, than I suppose that if something bad happens,
the recognition will fail.

Thanks,
Leehod.

Re: Question about merging two instructions.

[Paolo Bonzini]

>Do I always need to recognize the result?
>  
>
validate_change and apply_change_group will take care of that.

>If the answer is yes, than I suppose that if something bad happens,
>the recognition will fail.
>  
>
No, the problem is when recognition passes, and you have a SUBREG on the 
LHS that will only modify part of the pseudo.  I can't think off the top 
of mind of a case, but I'd rather err on the side of safety.

It may still make sense changing the default case of 
simplify_replace_rtx to invoke replace_rtx rather than returning x.  But 
this is unrelated, because nobody is currently passing a SET to 
simplify_replace_rtx (only expressions), and you should do the same: 
*you* said you want to replace on the RHS, so you really want to invoke 
simplify_replace_rtx on the RHS.

Paolo

Re: Question about merging two instructions.

[Richard Henderson]

On Mon, Aug 22, 2005 at 03:37:27PM +0200, Paolo Bonzini wrote:
> It may still make sense changing the default case of 
> simplify_replace_rtx to invoke replace_rtx rather than returning x.  But 
> this is unrelated, because nobody is currently passing a SET to 
> simplify_replace_rtx (only expressions), and you should do the same: 
> *you* said you want to replace on the RHS, so you really want to invoke 
> simplify_replace_rtx on the RHS.

Agreed.  If you're concerned about the LHS, use reg_overlap_mentioned_p
and abort the optimization.


r~

Re: Question about merging two instructions.

[Roger Sayle]

On Sun, 21 Aug 2005, Leehod Baruch wrote:

> >>(insn 1 0 2 0 (set (reg/v:Xmode r)
> >>        (sign_extend:Xmode (op:Ymode (...))))
> >>(insn 2 1 3 0 (set (lhs) (rhs)))
>
> 1. Can you please give me an example of something bad that can happen to
> the LHS.  Maybe I'm missing something here.

(set (reg:Xmode r) (sign_extend:Xmode (reg:Ymode p)))
(set (subreg:Ymode (reg:Xmode r) 0) (reg:Ymode q))

would be transfomed (by replacing all uses of "reg r" with it's
definition on both LHS and RHS) into:

(set (reg:Ymode p) (reg:Ymode q))


Originally, r's high part would be set to the signbit of p and r's low
part would be set to the value of q.  After the transformation, we now
overwrite the operand p with the value q, which isn't quite the same
thing.


> 2. After calling simplify_replace_rtx I try to recognize the instruction.
> Is this been cautious or is it unnecessary?

Except for register-register moves, all synthesized instructions need to
be rerecognized, especially after "RTL simplification".


> 3. Isn't it reasonable to expect that every instance on old_rtx will be
> replaced by new_rtx even if it can't be simplified?
> This is what I understand from the function's documentation.
> But actually every expressions that can't be simplified is not replaced.

Every instance of old_rtx should be replaced by new_rtx.  You may be
getting confused by the code to reduce memory usage.  If a replacement
doesn't occur within all operands/subtrees of a tree, then return this
tree. The invariant in the recursion is that if a substitution has been
made anywhere in the tree, it returns a newly allocated RTX.
Simplification of this newly allocated RTX, will itself return a newly
allocated RTX.

Hence the testing whether the return value of simplify_replace_rtx
matches it's original first argument is a way of determining whether
any substitution has been made (whether it was subsequently simplified
or not).


The one caveat to this is that simplify_replace_rtx is less robust
to unrecognized RTL codes than replace_rtx.  i.e. it won't traverse
UNSPECs or other non-unary/non-binary/non-comparison expressions.
This can/should probably be fixed by tweaking the "default:" case
to match the GET_RTX_FORMAT loop in replace_rtx.  Note this isn't
a simple cut'n'paste, as replace_rtx destructively overwrites it's
input expression, whilst simplify_replace_rtx returns a different
tree if anything changed.

Roger
--

Re: Question about merging two instructions.

[Paolo Bonzini]

>This approach seems reasonable.  The current structure of the code
>in simplify_replace_rtx is intended to handle RTL expressions rather
>than patterns, so normally it would be passed just SET_SRC (pat),
>instead of the whole set.
>  
>
Which is why, OTOH, I would be *extremely* cautious doing such a 
change.  Leehod said:

> > I tried to use simplify_replace_rtx to replace any use of (reg r) with[in]
> > the right-hand-side of the extension and simplify the result.

If he want to replace uses within the RHS of the extension, he should 
pass SET_SRC (pat).  He may as well want to handle parallels, in which 
case he should write a new function similar to this:

  int i;
  if (GET_CODE (pat) == SET)
    SET_SRC (pat) = simplify_replace_rtx (SET_SRC (pat), old, new);
  else if (GET_CODE (pat) == PARALLEL)
    for (i = 0; i < XVECLEN (pat, 0); i++)
      {
        rtx s = XVECEXP (pat, 0, i);
        if (GET_CODE (XVECEXP (pat, 0, i)) == SET)
          SET_SRC (s) = simplify_replace_rtx (SET_SRC (s), old, new);
      }

Paolo

Re: Question about merging two instructions.

[Leehod Baruch]

Paolo Bonzini <paolo.bonzini@lu.unisi.ch> wrote on 22/08/2005 10:10:40:
> > > I tried to use simplify_replace_rtx to replace any use of (reg r) 
with[in]
> > > the right-hand-side of the extension and simplify the result.
> 
> If he want to replace uses within the RHS of the extension, he should 
> pass SET_SRC (pat).  He may as well want to handle parallels, in which 
> case he should write a new function similar to this:

I think you misunderstood my original purpose. I did mean [with]
and not [in].
Let me explain again.

I have these two instructions:

(insn 1 0 2 0 (set (reg/v:Xmode r)
        (sign_extend:Xmode (op:Ymode (...))))
(insn 2 1 3 0 (set (LHS) (RHS)))

where:
1. Xmode > Ymode
2. RHS and/or LHS may contain: 
   (subreg:Ymode (reg/v:Xmode r) lowpart) and/or (reg/v:Xmode r).

Now I want to replace every *use* of (reg r) in insn 2 with the rhs of
insn 1 and simplify the result.  This is way the replacement may happen
in the LHS of insn 2.
Note that I don't want to replace any *def* and uses may appear in the 
LHS.

My plan was to use: replace_regs () to replace every use of (reg r)
with the a new pseudo register (because this is the only function that
I found that separates the uses from the defs) and then use
simplifiy_replace_rtx () to replace that new pseudo register with the rhs 
of insn 1 and simplify.

To make things even more complicated - insn 2 may be PARALLEL.

Maybe I should use simplify_rtx (replace_rtx (..))?
But it seem to me that simplify_rtx () doesn't deal with SET either.

Do you see a better way?

Thanks,
Leehod.

Re: Question about merging two instructions.

[Paolo Bonzini]

>Note that I don't want to replace any *def* and uses may appear in the 
>LHS.
>  
>
Ok, I see.  But you have to cope with *def*s appearing in the LHS: you 
don't want to replace them, yet your modified simplify_replace_rtx will!

>My plan was to use: replace_regs () to replace every use of (reg r)
>with the a new pseudo register (because this is the only function that
>I found that separates the uses from the defs)
>
Not really, as it will happily replace within a STRICT_LOW_PART, but 
that may as well be a bug.

I think you're best off making your own function for now.  And make it 
use validate_change so that you won't have to call recog manually.

Paolo