On Tue, May 11, 2021 at 11:59:24AM +0100, Richard Sandiford via Gcc-patches
wrote:
> > I wrote the following patch (originally against 10 branch because that is
> > where Uros has been debugging it) and bootstrapped/regtested it on 11
> > branch successfully.
> > It effectively implements your (2) above; I'm not sure if
> > REG_CAN_CHANGE_MODE_P is needed there, because it is already tested in
> > find_oldest_value_reg -> maybe_mode_change -> mode_change_ok.
>
> The REG_CAN_CHANGE_MODE_P test would in this case be for
> vd->e[dr].mode → vd->e[sr].mode, rather than oldest_regno's mode.
> I'm just worried that:
>
> (set (reg:HI R1) (reg:HI R0))
> (set (reg:SI R2) (reg:SI R1))
>
> isn't equivalent to:
>
> (set (reg:HI R1) (reg:HI R0))
> (set (reg:HI R2) (reg:HI R1))
>
> if REG_CAN_CHANGE_MODE_P is false for either the R2 or R1 change.
> If we pretend that it is when building the chain then there's a
> risk of GIGO when using it in find_oldest_value_reg.
>
> (Although in this case SI and HI are both valid for R1,
> REG_CAN_CHANGE_MODE_P might still be false if the HI bits are
> not in the low 16 bits of the SI. That's unlikely in this case,
> but a similar thing can happen for vector modes or multi-register modes.)
>
> I'm not saying the patch is wrong. I just wanted to clarify
> why I thought the check might be needed.
So, do you want something like (I've deleted the old comment as I think
the new one is enough, but am open to keep both) the patch below, where
it REG_CAN_CHANGE_MODE_P is false, we punt (return), otherwise call
set_value_regno?
Am not sure if those REG_CAN_CHANGE_MODE_P arguments is what you want
though.
--- gcc/regcprop.c.jj 2021-03-23 10:21:07.176447920 +0100
+++ gcc/regcprop.c 2021-05-13 17:31:39.940519855 +0200
@@ -358,34 +358,25 @@ copy_value (rtx dest, rtx src, struct va
else if (sn > hard_regno_nregs (sr, vd->e[sr].mode))
return;
- /* It is not safe to link DEST into the chain if SRC was defined in some
- narrower mode M and if M is also narrower than the mode of the first
- register in the chain. For example:
- (set (reg:DI r1) (reg:DI r0))
- (set (reg:HI r2) (reg:HI r1))
- (set (reg:SI r3) (reg:SI r2)) //Should be a new chain start at r3
- (set (reg:SI r4) (reg:SI r1))
- (set (reg:SI r5) (reg:SI r4))
-
- the upper part of r3 is undefined. If we added it to the chain,
- it may be used to replace r5, which has defined upper bits.
- See PR98694 for details.
-
- [A] partial_subreg_p (vd->e[sr].mode, GET_MODE (src))
- [B] partial_subreg_p (vd->e[sr].mode, vd->e[vd->e[sr].oldest_regno].mode)
- Condition B is added to to catch optimization opportunities of
-
- (set (reg:HI R1) (reg:HI R0))
- (set (reg:SI R2) (reg:SI R1)) // [A]
- (set (reg:DI R3) (reg:DI R2)) // [A]
- (set (reg:SI R4) (reg:SI R[0-3]))
- (set (reg:HI R5) (reg:HI R[0-4]))
-
- in which all registers have only 16 defined bits. */
- else if (partial_subreg_p (vd->e[sr].mode, GET_MODE (src))
- && partial_subreg_p (vd->e[sr].mode,
- vd->e[vd->e[sr].oldest_regno].mode))
- return;
+ /* If a narrower value is copied using wider mode, the upper bits
+ are undefined (could be e.g. a former paradoxical subreg). Signal
+ in that case we've only copied value using the narrower mode.
+ Consider:
+ (set (reg:DI r14) (mem:DI ...))
+ (set (reg:QI si) (reg:QI r14))
+ (set (reg:DI bp) (reg:DI r14))
+ (set (reg:DI r14) (const_int ...))
+ (set (reg:DI dx) (reg:DI si))
+ (set (reg:DI si) (const_int ...))
+ (set (reg:DI dx) (reg:DI bp))
+ The last set is not redundant, while the low 8 bits of dx are already
+ equal to low 8 bits of bp, the other bits are undefined. */
+ else if (partial_subreg_p (vd->e[sr].mode, GET_MODE (src)))
+ {
+ if (REG_CAN_CHANGE_MODE_P (sr, GET_MODE (src), vd->e[sr].mode))
+ return;
+ set_value_regno (dr, vd->e[sr].mode, vd);
+ }
/* Link DR at the end of the value chain used by SR. */
Jakub
