Patchwork [mips] Change the definition of macro SHIFT_COUNT_TRUNCATED

login
register
mail settings
Submitter Mingjie Xing
Date Aug. 28, 2010, 8:57 a.m.
Message ID <AANLkTin9Xh=16FET539SBzpj8XU4ySntqtmQxQcax9ux@mail.gmail.com>
Download mbox | patch
Permalink /patch/62899/
State New
Headers show

Comments

Mingjie Xing - Aug. 28, 2010, 8:57 a.m.
Hello,

This patch change the definition of macro SHIFT_COUNT_TRUNCATED. This
can fix the bug on Loongson for such an test case,

/* { dg-do run } */
/* { dg-options "isa=loongson -mhard-float -mno-mips16 -O1" } */

#include "loongson.h"
#include <assert.h>

typedef union { int32x2_t v; int32_t a[2]; } int32x2_encap_t;

void
main1 (int shift)
{
  int32x2_encap_t s;
  int32x2_encap_t r;

  s.a[0] = 0xffffffff;
  s.a[1] = 0xffffffff;
  /* Loongson SIMD use low-order 7 bits to specify the shift amount. Thus
   V2SI << 0x40 == 0. The below expression 'shift & 0x3f' will be mis-optimized
   as 'shift', if SHIFT_COUNT_TRUNCATED is nonzero. */
  r.v = psllw_s (s.v, (shift & 0x3f));
  assert (r.a[0] == 0xffffffff);
  assert (r.a[1] == 0xffffffff);
}

int
main (void)
{
  main1 (0x40);
  return 0;
}

ChangeLog:
2010-08-28  Mingjie Xing  <mingjie.xing@gmail.com>

        * config/mips/mips.h (SHIFT_COUNT_TRUNCATED) : Define to 0 for
Loongson targets, 1 otherwise.

Is it OK?

BTW, I'm not sure if I should commit the test case.

Thanks,
Mingjie
Richard Sandiford - Aug. 30, 2010, 5:43 p.m.
Mingjie Xing <mingjie.xing@gmail.com> writes:
> This patch change the definition of macro SHIFT_COUNT_TRUNCATED. This
> can fix the bug on Loongson for such an test case,
>
> /* { dg-do run } */
> /* { dg-options "isa=loongson -mhard-float -mno-mips16 -O1" } */
>
> #include "loongson.h"
> #include <assert.h>
>
> typedef union { int32x2_t v; int32_t a[2]; } int32x2_encap_t;
>
> void
> main1 (int shift)
> {
>   int32x2_encap_t s;
>   int32x2_encap_t r;
>
>   s.a[0] = 0xffffffff;
>   s.a[1] = 0xffffffff;
>   /* Loongson SIMD use low-order 7 bits to specify the shift amount. Thus
>    V2SI << 0x40 == 0. The below expression 'shift & 0x3f' will be mis-optimized
>    as 'shift', if SHIFT_COUNT_TRUNCATED is nonzero. */
>   r.v = psllw_s (s.v, (shift & 0x3f));
>   assert (r.a[0] == 0xffffffff);
>   assert (r.a[1] == 0xffffffff);
> }
>
> int
> main (void)
> {
>   main1 (0x40);
>   return 0;
> }
>
> ChangeLog:
> 2010-08-28  Mingjie Xing  <mingjie.xing@gmail.com>
>
>         * config/mips/mips.h (SHIFT_COUNT_TRUNCATED) : Define to 0 for
> Loongson targets, 1 otherwise.
>
> Is it OK?

You should also define TARGET_SHIFT_TRUNCATION_MASK.  We want to keep
the current behaviour of TARGET_SHIFT_TRUNCATION_MASK for non-vector
modes even when TARGET_LOONGSON is true.

Would you mind doing a simple check on a "real-life" body of code
to see how many times having SHIFT_COUNT_TRUNCATED set to 0 affects
optimisation of non-vector code?  Something like the linux kernel
would be good.  E.g. see how many linux .o files have different
text sections before and after the patch.

> BTW, I'm not sure if I should commit the test case.

Yeah, when the final patch goes in, please add the testcase too.
What you posted looks good, but please copy the comment:

/* loongson.h does not handle or check for MIPS16ness.  There doesn't
   seem any good reason for it to, given that the Loongson processors
   do not support MIPS16.  */

from loongson-simd.c, and put it above the dg-options line.  The way
we normally force non-MIPS16 mode is to add NOMIPS16 to the function
definition, but the comment is explaining why that doesn't work here.

Richard
Mingjie Xing - Sept. 1, 2010, 7:25 a.m.
2010/8/31 Richard Sandiford <rdsandiford@googlemail.com>:
> Would you mind doing a simple check on a "real-life" body of code
> to see how many times having SHIFT_COUNT_TRUNCATED set to 0 affects
> optimisation of non-vector code?  Something like the linux kernel
> would be good.  E.g. see how many linux .o files have different
> text sections before and after the patch.

Hi Richard, I made a test on linux kernel. There are 2640 .o files
generated by gcc and 419 files have difference in text sections after
having SHIFT_COUNT_TRUNCATED set to 0. The strange thing is that when
I also defined TARGET_SHIFT_TRUNCATION_MASK, changes still exist for
these 419 files. Seems that TARGET_SHIFT_TRUNCATION_MASK doesn't have
any effect. Here is the definition,

static unsigned HOST_WIDE_INT
mips_shift_truncation_mask (enum machine_mode mode)
{
  if (VECTOR_MODE_P (mode))
    return 0;

  return GET_MODE_BITSIZE (mode) - 1;
}

Regards,
Mingjie
Richard Sandiford - Sept. 1, 2010, 6 p.m.
Mingjie Xing <mingjie.xing@gmail.com> writes:
> 2010/8/31 Richard Sandiford <rdsandiford@googlemail.com>:
>> Would you mind doing a simple check on a "real-life" body of code
>> to see how many times having SHIFT_COUNT_TRUNCATED set to 0 affects
>> optimisation of non-vector code?  Something like the linux kernel
>> would be good.  E.g. see how many linux .o files have different
>> text sections before and after the patch.
>
> Hi Richard, I made a test on linux kernel. There are 2640 .o files
> generated by gcc and 419 files have difference in text sections after
> having SHIFT_COUNT_TRUNCATED set to 0.

Whoa.  That's a fair number.

> The strange thing is that when I also defined
> TARGET_SHIFT_TRUNCATION_MASK, changes still exist for these 419
> files. Seems that TARGET_SHIFT_TRUNCATION_MASK doesn't have any
> effect.

Probably not too surprising to be honest.  T_S_T_M only affects
expand, whereas SHIFT_COUNT_TRUNCATED affects the rtl optimisers.

It's far from ideal, but I suppose we have to go with it.
Can you post the complete patch?

Richard

Patch

Index: config/mips/mips.h
===================================================================
--- config/mips/mips.h	(revision 163612)
+++ config/mips/mips.h	(working copy)
@@ -2422,8 +2422,11 @@  typedef struct mips_args {
 #define SLOW_BYTE_ACCESS (!TARGET_MIPS16)
 
 /* Define this to be nonzero if shift instructions ignore all but the low-order
-   few bits.  */
-#define SHIFT_COUNT_TRUNCATED 1
+   few bits.
+
+   For Loongson targets, the Loongson specific vector shift instructions are
+   not SHIFT_COUNT_TRUNCATED. */
+#define SHIFT_COUNT_TRUNCATED (TARGET_LOONGSON_2EF ? 0 : 1)
 
 /* Value is 1 if truncating an integer of INPREC bits to OUTPREC bits
    is done just by pretending it is already truncated.  */