On Mon, Jul 22, 2024 at 08:16:16PM -0700, Andrew Pinski wrote:
> On Sun, Jul 21, 2024 at 11:47 PM Stefan Schulze Frielinghaus
> > diff --git a/gcc/regrename.cc b/gcc/regrename.cc
> > index 054e601740b..6ae5a2309d0 100644
> > --- a/gcc/regrename.cc
> > +++ b/gcc/regrename.cc
> > @@ -1113,6 +1113,10 @@ scan_rtx_reg (rtx_insn *insn, rtx *loc, enum
> > reg_class cl, enum scan_actions act
> >
> > c = create_new_chain (this_regno, this_nregs, loc, insn, cl);
> >
> > + /* Give up early in case of register pairs. */
> > + if (this_nregs != 1)
> > + c->cannot_rename = 1;
>
>
> I am a bit worried this will make TImode (and DImode for 32bit targets) worse.
> And it might make aarch64's vector struct types much worse than they
> are currently.
> It is interesting how there is a subreg of a hardregister after reload
> showing up here. Is that on purpose?
Good catch. I don't think this was on purpose. When looking at the
dump I rather thought this is valid RTL and didn't question it since
subregs for register pairs got "expanded" during final.
> They come from:
> ```
> (define_insn "*tf_to_fprx2_0"
> [(set (subreg:DF (match_operand:FPRX2 0 "nonimmediate_operand" "+f") 0)
> (subreg:DF (match_operand:TF 1 "general_operand" "v") 0))]
> ...
> (define_insn "*tf_to_fprx2_1"
> [(set (subreg:DF (match_operand:FPRX2 0 "nonimmediate_operand" "+f") 8)
> (subreg:DF (match_operand:TF 1 "general_operand" "v") 8))]
>
> ```
>
> I am not sure if that is a valid thing to do. s390 backend is the only
> one that has insn patterns like this. all that uses "+" use either
> strict_lowpart of zero_extract for the lhs or just a pure set.
> Maybe there is a better way of representing this. Maybe using unspec here?
I gave unspec a try and came up with
(define_insn "*tf_to_fprx2_0"
[(set (subreg:DF (match_operand:FPRX2 0 "nonimmediate_operand" "+f") 0)
(unspec:DF [(match_operand:TF 1 "general_operand" "v")]
UNSPEC_TF_TO_FPRX2_0))]
"TARGET_VXE"
; M4 == 1 corresponds to %v0[0] = %v1[0]; %v0[1] = %v0[1];
"vpdi\t%v0,%v1,%v0,1"
[(set_attr "op_type" "VRR")])
(define_insn "*tf_to_fprx2_1"
[(set (subreg:DF (match_operand:FPRX2 0 "nonimmediate_operand" "+f") 8)
(unspec:DF [(match_operand:TF 1 "general_operand" "v")]
UNSPEC_TF_TO_FPRX2_1))]
"TARGET_VXE"
; M4 == 5 corresponds to %V0[0] = %v1[1]; %V0[1] = %V0[1];
"vpdi\t%V0,%v1,%V0,5"
[(set_attr "op_type" "VRR")])
which seems to work. However, I'm still getting subregs at final:
(insn 3 18 7 (set (reg/v:TF 18 %f4 [orig:62 x ] [62])
(mem/c:TF (reg:DI 2 %r2 [65]) [1 x+0 S16 A64])) "t.c":3:1 421 {movtf_vr}
(expr_list:REG_DEAD (reg:DI 2 %r2 [65])
(nil)))
(insn 7 3 8 (set (subreg:DF (reg:FPRX2 16 %f0 [64]) 0)
(unspec:DF [
(reg/v:TF 18 %f4 [orig:62 x ] [62])
] UNSPEC_TF_TO_FPRX2_0)) "t.c":4:10 569 {*tf_to_fprx2_0}
(nil))
(insn 8 7 14 (set (subreg:DF (reg:FPRX2 16 %f0 [64]) 8)
(unspec:DF [
(reg/v:TF 18 %f4 [orig:62 x ] [62])
] UNSPEC_TF_TO_FPRX2_1)) "t.c":4:10 570 {*tf_to_fprx2_1}
(expr_list:REG_DEAD (reg/v:TF 18 %f4 [orig:62 x ] [62])
(nil)))
Thus, I'm not sure whether this really solves the problem or rather
shifts around it. I'm still a bit puzzled why the initial RTL is
invalid. If I understood you correctly Jeff, then we are missing a
pattern which would match once the subregs are eliminated. Since none
exists the subregs survive and regrename gets confused. This basically
means that subregs of register pairs must not survive RA and the unspec
solution from above is no real solution.
Since the only purpose of tf_to_fprx2_0 and tf_to_fprx2_1 are to move a
long double from a vector register into a FP register pair one could
also merge both insn into one and emit two instructions in the assembler
template. This would at least circumvent the subreg issue.
(define_insn "tf_to_fprx2"
[(set (match_operand:FPRX2 0 "nonimmediate_operand" "=f")
(unspec:FPRX2 [(match_operand:TF 1 "general_operand" "v")]
UNSPEC_TF_TO_FPRX2))]
"TARGET_VXE"
"vpdi\t%v0,%v1,%v0,1;vpdi\t%V0,%v1,%V0,5"
[(set_attr "length" "12")
(set_attr "op_type" "VRR")])
I will give this a try tomorrow.
Thanks,
Stefan
