[PATCH] Add -meb to two Octeon tests

[PATCH] Add -meb to two Octeon tests

[Adam Nemet]

There are two tests that fail when compiled for little-endian.

octeon-exts-2.c is more obvious.  In this structure:

struct bar 
{ 
  unsigned long long a:1; 
  long long b:14; 
  unsigned long long c:48; 
  long long d:1; 
}; 

when d is extracted, it is the bottom bit in big-endian but the top bit in
little-endian.  If d is the top bit then signed extraction can be achieved
equally well with a arithmetic right shift.

In octeon-bbit-3.c -- which is really endian-agnostic --- for both bit
comparisons a different insn is generated by combine than what we have bbit
patterns for:

(if_then_else (ne (zero_extract:DI (subreg:DI (truncate:SI (reg:DI 196)) 0)
                (const_int 1)
                (const_int 0))
            (const_int 0))
        (label_ref 20)
        (pc)))

There is a pattern for this in our compiler but this is something the
middle-end should be able to simplify.  I added this to my todo list.

I would prefer changing this test to big-endian-only as well.  My intent was
to guard us from regressing in this area and not to add tests that are known
to fail.

Now all octeon*.c tests pass with -mel with all three ABIs.

OK to install?

Adam

	* gcc.target/mips/octeon-exts-2.c: Compile with -meb.
	* gcc.target/mips/octeon-bbit-3.c: Likewise.

Index: octeon-exts-2.c
===================================================================
--- octeon-exts-2.c	(revision 140670)
+++ octeon-exts-2.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-mips-options "-O -march=octeon" } */
+/* { dg-mips-options "-O -march=octeon -meb" } */
 /* { dg-final { scan-assembler-times "\texts\t" 4 } } */
 
 struct bar
Index: octeon-bbit-3.c
===================================================================
--- octeon-bbit-3.c	(revision 140670)
+++ octeon-bbit-3.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-mips-options "-O2 -march=octeon" } */
+/* { dg-mips-options "-O2 -march=octeon -meb" } */
 /* { dg-final { scan-assembler-times "\tbbit\[01\]\t|\tbgez\t" 2 } } */
 /* { dg-final { scan-assembler-not "ext\t" } } */
 

Re: [PATCH] Add -meb to two Octeon tests

[Richard Sandiford]

Adam Nemet <anemet@caviumnetworks.com> writes:
> There are two tests that fail when compiled for little-endian.
>
> octeon-exts-2.c is more obvious.  In this structure:
>
> struct bar 
> { 
>   unsigned long long a:1; 
>   long long b:14; 
>   unsigned long long c:48; 
>   long long d:1; 
> }; 
>
> when d is extracted, it is the bottom bit in big-endian but the top bit in
> little-endian.  If d is the top bit then signed extraction can be achieved
> equally well with a arithmetic right shift.

Since we're still supposed to be able to use this instruction for
_some_ code on little endian targets, I'd prefer that we either:

  (a) apply your patch but create an -mel-friendly version too.
  (b) reverse the order of the fields when _MIPSEL is defined.

The first is better really.  (I know it must seem daft having
-mel Octeon tests, but it isn't a rejected combination.)

> In octeon-bbit-3.c -- which is really endian-agnostic --- for both bit
> comparisons a different insn is generated by combine than what we have bbit
> patterns for:
>
> (if_then_else (ne (zero_extract:DI (subreg:DI (truncate:SI (reg:DI 196)) 0)
>                 (const_int 1)
>                 (const_int 0))
>             (const_int 0))
>         (label_ref 20)
>         (pc)))
>
> There is a pattern for this in our compiler but this is something the
> middle-end should be able to simplify.  I added this to my todo list.
>
> I would prefer changing this test to big-endian-only as well.  My intent was
> to guard us from regressing in this area and not to add tests that are known
> to fail.

That's fine, but please put something like the above in a comment.

Richard

Re: [PATCH] Add -meb to two Octeon tests

[Adam Nemet]

Richard Sandiford writes:
> Adam Nemet <anemet@caviumnetworks.com> writes:
> > There are two tests that fail when compiled for little-endian.
> >
> > octeon-exts-2.c is more obvious.  In this structure:
> >
> > struct bar 
> > { 
> >   unsigned long long a:1; 
> >   long long b:14; 
> >   unsigned long long c:48; 
> >   long long d:1; 
> > }; 
> >
> > when d is extracted, it is the bottom bit in big-endian but the top bit in
> > little-endian.  If d is the top bit then signed extraction can be achieved
> > equally well with a arithmetic right shift.
> 
> Since we're still supposed to be able to use this instruction for
> _some_ code on little endian targets, I'd prefer that we either:
> 
>   (a) apply your patch but create an -mel-friendly version too.
>   (b) reverse the order of the fields when _MIPSEL is defined.
> 
> The first is better really.  (I know it must seem daft having
> -mel Octeon tests, but it isn't a rejected combination.)

Since one of the cases in this test is meant to verify extraction of the
bottom bit I went with (b) here.  (Also note that there are other exts tests
already that are -mel-friendly.)

> > In octeon-bbit-3.c -- which is really endian-agnostic --- for both bit
> > comparisons a different insn is generated by combine than what we have bbit
> > patterns for:
> >
> > (if_then_else (ne (zero_extract:DI (subreg:DI (truncate:SI (reg:DI 196)) 0)
> >                 (const_int 1)
> >                 (const_int 0))
> >             (const_int 0))
> >         (label_ref 20)
> >         (pc)))
> >
> > There is a pattern for this in our compiler but this is something the
> > middle-end should be able to simplify.  I added this to my todo list.
> >
> > I would prefer changing this test to big-endian-only as well.  My intent was
> > to guard us from regressing in this area and not to add tests that are known
> > to fail.
> 
> That's fine, but please put something like the above in a comment.

Sure.  Here is the updated patch.

Tested these testcases with {,-mabi=32,-mabi=n32,-mabi=64,-meb,-mel,-mel\
-mabi=32} on mips64octeon-linux-gnu.

OK to install?

Adam

	* gcc.target/mips/octeon-exts-2.c: Make it work with -mel.
	* gcc.target/mips/octeon-bbit-3.c: Compile with -meb.  Add
	comment why this is necessary.

Index: gcc.target/mips/octeon-exts-2.c
===================================================================
--- gcc.target/mips/octeon-exts-2.c	(revision 140670)
+++ gcc.target/mips/octeon-exts-2.c	(working copy)
@@ -4,10 +4,17 @@
 
 struct bar
 {
+#ifdef _MIPSEB
   unsigned long long a:1;
   long long b:14;
   unsigned long long c:48;
   long long d:1;
+#else
+  long long d:1;
+  unsigned long long c:48;
+  long long b:14;
+  unsigned long long a:1;
+#endif
 };
 
 NOMIPS16 int
Index: gcc.target/mips/octeon-bbit-3.c
===================================================================
--- gcc.target/mips/octeon-bbit-3.c	(revision 140670)
+++ gcc.target/mips/octeon-bbit-3.c	(working copy)
@@ -1,5 +1,18 @@
 /* { dg-do compile } */
-/* { dg-mips-options "-O2 -march=octeon" } */
+
+/* Force big-endian because for little-endian, combine generates this:
+
+ (if_then_else (ne (zero_extract:DI (subreg:DI (truncate:SI (reg:DI 196)) 0) 
+                 (const_int 1) 
+                 (const_int 0)) 
+             (const_int 0)) 
+         (label_ref 20) 
+         (pc))) 
+
+  which does not get recognized as a valid bbit pattern.  The
+  middle-end should be able to simplify this futher.  */
+/* { dg-mips-options "-O2 -march=octeon -meb" } */
+
 /* { dg-final { scan-assembler-times "\tbbit\[01\]\t|\tbgez\t" 2 } } */
 /* { dg-final { scan-assembler-not "ext\t" } } */
 

Re: [PATCH] Add -meb to two Octeon tests

[Richard Sandiford]

Adam Nemet <anemet@caviumnetworks.com> writes:
> Richard Sandiford writes:
>> Adam Nemet <anemet@caviumnetworks.com> writes:
>> > There are two tests that fail when compiled for little-endian.
>> >
>> > octeon-exts-2.c is more obvious.  In this structure:
>> >
>> > struct bar 
>> > { 
>> >   unsigned long long a:1; 
>> >   long long b:14; 
>> >   unsigned long long c:48; 
>> >   long long d:1; 
>> > }; 
>> >
>> > when d is extracted, it is the bottom bit in big-endian but the top bit in
>> > little-endian.  If d is the top bit then signed extraction can be achieved
>> > equally well with a arithmetic right shift.
>> 
>> Since we're still supposed to be able to use this instruction for
>> _some_ code on little endian targets, I'd prefer that we either:
>> 
>>   (a) apply your patch but create an -mel-friendly version too.
>>   (b) reverse the order of the fields when _MIPSEL is defined.
>> 
>> The first is better really.  (I know it must seem daft having
>> -mel Octeon tests, but it isn't a rejected combination.)
>
> Since one of the cases in this test is meant to verify extraction of the
> bottom bit I went with (b) here.  (Also note that there are other exts tests
> already that are -mel-friendly.)

I'm not sure from your reply whether this was clear or not, so just to
be on the safe side: the idea with (a) was to copy the test to a new
file and replace the structure definition with the _MIPSEL version.
The old test would then require -meb and the new one would require -mel.
The idea is that someone testing without endianness options (which is
the usual case on IRIX and GNU/Linux) would still end up running both
the -meb and -mel tests.

> 	* gcc.target/mips/octeon-exts-2.c: Make it work with -mel.
> 	* gcc.target/mips/octeon-bbit-3.c: Compile with -meb.  Add
> 	comment why this is necessary.

OK, thanks, but:

> +  middle-end should be able to simplify this futher.  */

s/futher/further/

Richard

Re: [PATCH] Add -meb to two Octeon tests

[Adam Nemet]

Richard Sandiford writes:
> Adam Nemet <anemet@caviumnetworks.com> writes:
> > Richard Sandiford writes:
> >> Adam Nemet <anemet@caviumnetworks.com> writes:
> >> > There are two tests that fail when compiled for little-endian.
> >> >
> >> > octeon-exts-2.c is more obvious.  In this structure:
> >> >
> >> > struct bar 
> >> > { 
> >> >   unsigned long long a:1; 
> >> >   long long b:14; 
> >> >   unsigned long long c:48; 
> >> >   long long d:1; 
> >> > }; 
> >> >
> >> > when d is extracted, it is the bottom bit in big-endian but the top bit in
> >> > little-endian.  If d is the top bit then signed extraction can be achieved
> >> > equally well with a arithmetic right shift.
> >> 
> >> Since we're still supposed to be able to use this instruction for
> >> _some_ code on little endian targets, I'd prefer that we either:
> >> 
> >>   (a) apply your patch but create an -mel-friendly version too.
> >>   (b) reverse the order of the fields when _MIPSEL is defined.
> >> 
> >> The first is better really.  (I know it must seem daft having
> >> -mel Octeon tests, but it isn't a rejected combination.)
> >
> > Since one of the cases in this test is meant to verify extraction of the
> > bottom bit I went with (b) here.  (Also note that there are other exts tests
> > already that are -mel-friendly.)
> 
> I'm not sure from your reply whether this was clear or not, so just to
> be on the safe side: the idea with (a) was to copy the test to a new
> file and replace the structure definition with the _MIPSEL version.
> The old test would then require -meb and the new one would require -mel.
> The idea is that someone testing without endianness options (which is
> the usual case on IRIX and GNU/Linux) would still end up running both
> the -meb and -mel tests.

Yes, I did misunderstand.  I thought that when you said '-mel-friendly' you
meant endian agnostic.  Here is the patch to split the test along your idea.
You're right, it clearly provides better coverage.

> s/futher/further/

Done.

Retested the octeon-exts-[25] tests with
{,-mabi=32,-mabi=n32,-mabi=64,-meb,-mel,-mel\ -mabi=32,-mips16\ -mabi=32}.

OK?

	* gcc.target/mips/octeon-exts-2.c: Compile it with -meb.
	* gcc.target/mips/octeon-exts-5.c: New test.
	* gcc.target/mips/octeon-bbit-3.c: Compile with -meb.  Add
	comment why this is necessary.

Index: gcc.target/mips/octeon-exts-2.c
===================================================================
--- gcc.target/mips/octeon-exts-2.c	(revision 140961)
+++ gcc.target/mips/octeon-exts-2.c	(working copy)
@@ -1,5 +1,5 @@
 /* { dg-do compile } */
-/* { dg-mips-options "-O -march=octeon" } */
+/* { dg-mips-options "-O -march=octeon -meb" } */
 /* { dg-final { scan-assembler-times "\texts\t" 4 } } */
 
 struct bar
Index: gcc.target/mips/octeon-exts-5.c
===================================================================
--- gcc.target/mips/octeon-exts-5.c	(revision 0)
+++ gcc.target/mips/octeon-exts-5.c	(revision 0)
@@ -0,0 +1,38 @@
+/* -mel version of octeon-exts-2.c.  */
+/* { dg-do compile } */
+/* { dg-mips-options "-O -march=octeon -mel" } */
+/* { dg-final { scan-assembler-times "\texts\t" 4 } } */
+
+struct bar
+{
+  long long d:1;
+  unsigned long long c:48;
+  long long b:14;
+  unsigned long long a:1;
+};
+
+NOMIPS16 int
+f1 (struct bar *s, int a)
+{
+  return (int) s->b + a;
+}
+
+NOMIPS16 char
+f2 (struct bar *s)
+{
+  return s->d + 1;
+}
+
+NOMIPS16 int
+f3 ()
+{
+  struct bar s;
+  asm ("" : "=r"(s));
+  return (int) s.b + 1;
+}
+
+NOMIPS16 long long
+f4 (struct bar *s)
+{
+  return s->d;
+}
Index: gcc.target/mips/octeon-bbit-3.c
===================================================================
--- gcc.target/mips/octeon-bbit-3.c	(revision 140961)
+++ gcc.target/mips/octeon-bbit-3.c	(working copy)
@@ -1,5 +1,18 @@
 /* { dg-do compile } */
-/* { dg-mips-options "-O2 -march=octeon" } */
+
+/* Force big-endian because for little-endian, combine generates this:
+
+ (if_then_else (ne (zero_extract:DI (subreg:DI (truncate:SI (reg:DI 196)) 0) 
+                 (const_int 1) 
+                 (const_int 0)) 
+             (const_int 0)) 
+         (label_ref 20) 
+         (pc))) 
+
+  which does not get recognized as a valid bbit pattern.  The
+  middle-end should be able to simplify this further.  */
+/* { dg-mips-options "-O2 -march=octeon -meb" } */
+
 /* { dg-final { scan-assembler-times "\tbbit\[01\]\t|\tbgez\t" 2 } } */
 /* { dg-final { scan-assembler-not "ext\t" } } */

Re: [PATCH] Add -meb to two Octeon tests

[Richard Sandiford]

Thanks for the update, and for your patience ;)

Adam Nemet <anemet@caviumnetworks.com> writes:
> 	* gcc.target/mips/octeon-exts-2.c: Compile it with -meb.
> 	* gcc.target/mips/octeon-exts-5.c: New test.
> 	* gcc.target/mips/octeon-bbit-3.c: Compile with -meb.  Add
> 	comment why this is necessary.

OK.

Richard