optabs: Variable index vec_set

optabs: Variable index vec_set

[Robin Dapp]

Hi,

I'm looking into vec_set with variable index on s390.  Uros posted a
patch [1] that did not make it upstream in Nov 2020.  It changed the
mode of the index operand to whatever the target supports in
can_vec_set_var_idx_p.  I missed it back then but we indeed do not make
proper use of vec_set with an index register.

With the patch my local changes to make better use of vec_set work
nicely even though I haven't done a full bootstrap yet.  Were there
other issues with the patch or can it still be applied?

Regards
 Robin

[1]
https://gcc.gnu.org/pipermail/gcc-patches/2020-November/559213.html

Re: optabs: Variable index vec_set

[Robin Dapp]

Hi,

> With the patch my local changes to make better use of vec_set work
> nicely even though I haven't done a full bootstrap yet.  Were there
> other issues with the patch or can it still be applied?

I performed a bootstrap as well as a regtest with -march=z16 on s390.
There is no new fallout.

Regards
 Robin

Re: optabs: Variable index vec_set

[Uros Bizjak]

On Wed, Nov 2, 2022 at 1:12 PM Robin Dapp <rdapp@linux.ibm.com> wrote:
>
> Hi,
>
> > With the patch my local changes to make better use of vec_set work
> > nicely even though I haven't done a full bootstrap yet.  Were there
> > other issues with the patch or can it still be applied?
>
> I performed a bootstrap as well as a regtest with -march=z16 on s390.
> There is no new fallout.

IIRC, I was trying to "fix" modeless operand by giving it a mode, but
since it made no difference for x86, I later dropped the patch.
However, operand with a known mode is preferred, so if it works for
you, just include my patch in your submission. My patch is somehow
trivial if we want operand to have known mode.

Uros.

Re: optabs: Variable index vec_set

[Robin Dapp]

> IIRC, I was trying to "fix" modeless operand by giving it a mode, but
> since it made no difference for x86, I later dropped the patch.
> However, operand with a known mode is preferred, so if it works for
> you, just include my patch in your submission. My patch is somehow
> trivial if we want operand to have known mode.

I'd prefer to push it separately as my patch changes several things in
the s390 backend that are kind of unrelated.  Is it OK to do an x86
bootstrap and regtest and push it if everything looks good?  You can of
course also do it yourself :)

Thanks
 Robin

Re: optabs: Variable index vec_set

[Uros Bizjak]

On Wed, Nov 2, 2022 at 1:45 PM Robin Dapp <rdapp@linux.ibm.com> wrote:
>
> > IIRC, I was trying to "fix" modeless operand by giving it a mode, but
> > since it made no difference for x86, I later dropped the patch.
> > However, operand with a known mode is preferred, so if it works for
> > you, just include my patch in your submission. My patch is somehow
> > trivial if we want operand to have known mode.
>
> I'd prefer to push it separately as my patch changes several things in
> the s390 backend that are kind of unrelated.  Is it OK to do an x86
> bootstrap and regtest and push it if everything looks good?  You can of
> course also do it yourself :)

It is a middle-end patch, someone will have to approve it.

Uros.

Re: optabs: Variable index vec_set

[Richard Biener]

On Wed, Nov 2, 2022 at 1:46 PM Uros Bizjak <ubizjak@gmail.com> wrote:
>
> On Wed, Nov 2, 2022 at 1:45 PM Robin Dapp <rdapp@linux.ibm.com> wrote:
> >
> > > IIRC, I was trying to "fix" modeless operand by giving it a mode, but
> > > since it made no difference for x86, I later dropped the patch.
> > > However, operand with a known mode is preferred, so if it works for
> > > you, just include my patch in your submission. My patch is somehow
> > > trivial if we want operand to have known mode.
> >
> > I'd prefer to push it separately as my patch changes several things in
> > the s390 backend that are kind of unrelated.  Is it OK to do an x86
> > bootstrap and regtest and push it if everything looks good?  You can of
> > course also do it yourself :)
>
> It is a middle-end patch, someone will have to approve it.

The patch is OK

> Uros.

Re: optabs: Variable index vec_set

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

On Sat, Nov 5, 2022 at 12:25 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Wed, Nov 2, 2022 at 1:46 PM Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > On Wed, Nov 2, 2022 at 1:45 PM Robin Dapp <rdapp@linux.ibm.com> wrote:
> > >
> > > > IIRC, I was trying to "fix" modeless operand by giving it a mode, but
> > > > since it made no difference for x86, I later dropped the patch.
> > > > However, operand with a known mode is preferred, so if it works for
> > > > you, just include my patch in your submission. My patch is somehow
> > > > trivial if we want operand to have known mode.
> > >
> > > I'd prefer to push it separately as my patch changes several things in
> > > the s390 backend that are kind of unrelated.  Is it OK to do an x86
> > > bootstrap and regtest and push it if everything looks good?  You can of
> > > course also do it yourself :)
> >
> > It is a middle-end patch, someone will have to approve it.
>
> The patch is OK

Thanks, pushed with the following ChangeLog:

optabs: Use operand[2] mode in can_vec_set_var_idx_p

Use operand[2] mode in can_vec_set_var_idx_p when checking vec_set_optab.

This change allows non-VOID index operand in vec_set_optab.

2022-11-06  Uroš Bizjak  <ubizjak@gmail.com>

gcc/ChangeLog:

    * optabs.cc (can_vec_set_var_idx_p): Use operand[2]
    mode when checking vec_set_optab.

Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.

Uros.

[-- Attachment #2: p.diff.txt --]
[-- Type: text/plain, Size: 890 bytes --]

diff --git a/gcc/optabs.cc b/gcc/optabs.cc
index c2a6f971d74..9fc9b1fc6e9 100644
--- a/gcc/optabs.cc
+++ b/gcc/optabs.cc
@@ -4344,12 +4344,17 @@ can_vec_set_var_idx_p (machine_mode vec_mode)
     return false;
 
   machine_mode inner_mode = GET_MODE_INNER (vec_mode);
+
   rtx reg1 = alloca_raw_REG (vec_mode, LAST_VIRTUAL_REGISTER + 1);
   rtx reg2 = alloca_raw_REG (inner_mode, LAST_VIRTUAL_REGISTER + 2);
-  rtx reg3 = alloca_raw_REG (VOIDmode, LAST_VIRTUAL_REGISTER + 3);
 
   enum insn_code icode = optab_handler (vec_set_optab, vec_mode);
 
+  const struct insn_data_d *data = &insn_data[icode];
+  machine_mode idx_mode = data->operand[2].mode;
+
+  rtx reg3 = alloca_raw_REG (idx_mode, LAST_VIRTUAL_REGISTER + 3);
+
   return icode != CODE_FOR_nothing && insn_operand_matches (icode, 0, reg1)
 	 && insn_operand_matches (icode, 1, reg2)
 	 && insn_operand_matches (icode, 2, reg3);