On Tue, Jul 23, 2024 at 11:40:00AM -0600, Jeff Law wrote:
>
>
> On 7/23/24 9:45 AM, Stefan Schulze Frielinghaus wrote:
>
> >
> > > 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.
> I'd tend to agree. The routine in question is cleanup_subreg_operands and
> from a quick looksie it's not going to work for the insn in question because
> cleanup_subreg_operands actually looks down into the recog data structures
> for each operand. In the case above the subreg is explicit in the RTL
> rather than matched by the operand predicate.
Right, I did some further tests over night where I also added patterns
in order to match variants where the subregs are eliminated and that
seems to work. I still haven't made up my mind which route would be
best. Anyhow, it is clear that this patch should be dropped and I will
come up with a solution for the target.
Thank you Andrew and Jeff for pointing this out. Some myths about
subregs have been revealed for me :)
Cheers,
Stefan
