From: Bill Schmidt <wschmidt@linux.vnet.ibm.com>
To: Segher Boessenkool <segher@kernel.crashing.org>
Cc: Michael Meissner <meissner@linux.vnet.ibm.com>,
GCC Patches <gcc-patches@gcc.gnu.org>,
David Edelsohn <dje.gcc@gmail.com>
Subject: Re: [PATCH], PR target/81593, Optimize PowerPC vector sets coming from a vector extracts
Date: Sun, 30 Jul 2017 14:01:00 -0000 [thread overview]
Message-ID: <E64CEC2D-FC94-4769-A07F-210FA10DED6B@linux.vnet.ibm.com> (raw)
In-Reply-To: <20170728210848.GC13471@gate.crashing.org>
> On Jul 28, 2017, at 4:08 PM, Segher Boessenkool <segher@kernel.crashing.org> wrote:
>
> Hi!
>
> On Thu, Jul 27, 2017 at 07:21:14PM -0400, Michael Meissner wrote:
>> This patches optimizes the PowerPC vector set operation for 64-bit doubles and
>> longs where the elements in the vector set may have been extracted from another
>> vector (PR target/81593):
>
>> * config/rs6000/rs6000.c (rs6000_emit_xxpermdi): New function to
>> emit XXPERMDI accessing either double word in either vector
>> register inputs.
>
> "emit" is not a good name for this: that is generally used for something
> that does emit_insn, i.e. put an insn in the instruction stream. This
> function returns a string a define_insn can return. For the rl* insns
> I called the similar functions rs6000_insn_for_*, maybe something like
> that is better here?
>
>> +/* Emit a XXPERMDI instruction that can extract from either double word of the
>> + two arguments. ELEMENT1 and ELEMENT2 are either NULL or they are 0/1 giving
>> + which double word to be used for the operand. */
>> +
>> +const char *
>> +rs6000_emit_xxpermdi (rtx operands[], rtx element1, rtx element2)
>> +{
>> + int op1_dword = (!element1) ? 0 : INTVAL (element1);
>> + int op2_dword = (!element2) ? 0 : INTVAL (element2);
>> +
>> + gcc_assert (IN_RANGE (op1_dword | op2_dword, 0, 1));
>> +
>> + if (BYTES_BIG_ENDIAN)
>> + {
>> + operands[3] = GEN_INT (2*op1_dword + op2_dword);
>> + return "xxpermdi %x0,%x1,%x2,%3";
>> + }
>> + else
>> + {
>> + if (element1)
>> + op1_dword = 1 - op1_dword;
>> +
>> + if (element2)
>> + op2_dword = 1 - op2_dword;
>> +
>> + operands[3] = GEN_INT (op1_dword + 2*op2_dword);
>> + return "xxpermdi %x0,%x2,%x1,%3";
>> + }
>> +}
>
> I think calling this with the rtx elementN args makes this only more
> complicated (the function comment doesn't say what they are or what
> NULL means, btw).
>
>> (define_insn "vsx_concat_<mode>"
>> - [(set (match_operand:VSX_D 0 "gpc_reg_operand" "=<VSa>,we")
>> + [(set (match_operand:VSX_D 0 "vsx_register_operand" "=wa,we")
>> (vec_concat:VSX_D
>> - (match_operand:<VS_scalar> 1 "gpc_reg_operand" "<VS_64reg>,b")
>> - (match_operand:<VS_scalar> 2 "gpc_reg_operand" "<VS_64reg>,b")))]
>> + (match_operand:<VS_scalar> 1 "gpc_reg_operand" "wa,b")
>> + (match_operand:<VS_scalar> 2 "gpc_reg_operand" "wa,b")))]
>> "VECTOR_MEM_VSX_P (<MODE>mode)"
>> {
>> if (which_alternative == 0)
>> - return (BYTES_BIG_ENDIAN
>> - ? "xxpermdi %x0,%x1,%x2,0"
>> - : "xxpermdi %x0,%x2,%x1,0");
>> + return rs6000_emit_xxpermdi (operands, NULL_RTX, NULL_RTX);
>>
>> else if (which_alternative == 1)
>> - return (BYTES_BIG_ENDIAN
>> + return (VECTOR_ELT_ORDER_BIG
>> ? "mtvsrdd %x0,%1,%2"
>> : "mtvsrdd %x0,%2,%1");
>
> This one could be
>
> {
> if (!BYTES_BIG_ENDIAN)
!VECTOR_ELT_ORDER_BIG (to accommodate -maltivec=be). (We have some general bitrot associated with -maltivec=be that needs to be addressed, or we need to give up on it altogether. Still of two minds about this.)
Bill
> std::swap (operands[1], operands[2]);
>
> switch (which_alternative)
> {
> case 0:
> return "xxpermdi %x0,%x1,%x2,0";
> case 1:
> return "mtvsrdd %x0,%1,%2";
> default:
> gcc_unreachable ();
> }
> }
>
> (Could/should we use xxmrghd instead? Do all supported assemblers know
> that extended mnemonic, is it actually more readable?)
>
>> --- gcc/testsuite/gcc.target/powerpc/vsx-extract-7.c (svn+ssh://meissner@gcc.gnu.org/svn/gcc/trunk/gcc/testsuite/gcc.target/powerpc/vsx-extract-7.c) (revision 0)
>> +++ gcc/testsuite/gcc.target/powerpc/vsx-extract-7.c (.../gcc/testsuite/gcc.target/powerpc/vsx-extract-7.c) (revision 250640)
>> @@ -0,0 +1,15 @@
>> +/* { dg-do compile { target { powerpc*-*-* } } } */
>> +/* { dg-skip-if "" { powerpc*-*-darwin* } } */
>> +/* { dg-require-effective-target powerpc_vsx_ok } */
>> +/* { dg-options "-O2 -mvsx" } */
>> +
>> +vector double
>> +test_vpasted (vector double high, vector double low)
>> +{
>> + vector double res;
>> + res[1] = high[1];
>> + res[0] = low[0];
>> + return res;
>> +}
>> +
>> +/* { dg-final { scan-assembler-times {\mxxpermdi\M} 1 } } */
>
> In this and the other testcase, should you test no other insns at all
> are generated?
>
>
> Segher
>
next prev parent reply other threads:[~2017-07-30 14:01 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-27 23:21 Michael Meissner
2017-07-28 7:51 ` Richard Biener
2017-07-28 8:02 ` Andrew Pinski
2017-07-28 8:20 ` Richard Biener
2017-07-28 8:42 ` Marc Glisse
2017-07-28 14:44 ` Michael Meissner
2017-07-28 21:09 ` Segher Boessenkool
2017-07-30 14:01 ` Bill Schmidt [this message]
2017-07-31 17:41 ` Michael Meissner
2017-07-31 17:42 ` Bill Schmidt
2017-07-31 17:38 ` Michael Meissner
2017-08-02 14:29 ` Michael Meissner
2017-08-03 15:01 ` Segher Boessenkool
2017-08-03 17:23 ` Michael Meissner
2017-08-07 13:18 ` Michael Meissner
2017-08-07 21:54 ` Segher Boessenkool
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=E64CEC2D-FC94-4769-A07F-210FA10DED6B@linux.vnet.ibm.com \
--to=wschmidt@linux.vnet.ibm.com \
--cc=dje.gcc@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=meissner@linux.vnet.ibm.com \
--cc=segher@kernel.crashing.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).