diff mbox

[AVR] : Built-in for non-contiguous port layouts

Message ID 4F3933D9.7050308@gjlay.de
State New
Headers show

Commit Message

Georg-Johann Lay Feb. 13, 2012, 4:01 p.m. UTC 
This patch set removes __builtin_avr_map8 __builtin_avr_map16 built-ins and
implements a built-in __builtin_avr_insert_bits instead.

This has several reasons:

* From user feedback I learned that speed matters more than size here

* I found that the new built-in has better usability and fits better to
  the intended use cases.

* Better code is generated by implementing hook TARGET_FOLD_BUILTIN.

* The implementation is simpler (except the new folding part).

* There were issues with __builtin_avr_map*.  Instead of fixing these
  I went ahead an removed them altogether

* The new built-in is generic enough to provide the old ones'
  functionalities easily.

There are 2 new test programs for this built-in that all pass fine.

Ok for trunk?

Johann


gcc/doc/
	* extend.texi (AVR Built-in Functions): Remove doc for
	__builtin_avr_map8, __builtin_avr_map16.
	Document __builtin_avr_insert_bits.

gcc/testsuite/
	* gcc.target/avr/torture/builtin_insert_bits-1.c: New test.
	* gcc.target/avr/torture/builtin_insert_bits-2.c: New test.

gcc/
	* config/avr/avr.md (map_bitsqi, map_bitshi): Remove.
	(insert_bits): New insn.
	(adjust_len.map_bits): Rename to insert_bits.
	(UNSPEC_MAP_BITS): Rename to UNSPEC_INSERT_BITS.

	* avr-protos.h (avr_out_map_bits): Remove.
	(avr_out_insert_bits, avr_has_nibble_0xf): New.

	* config/avr/constraints.md (Cxf,C0f): New.

	* config/avr/avr.c (avr_cpu_cpp_builtins): Remove built-in
	defines __BUILTIN_AVR_MAP8, __BUILTIN_AVR_MAP16.
	New built-in define __BUILTIN_AVR_INSERT_BITS.

	* config/avr/avr.c (TARGET_FOLD_BUILTIN): New define.
	(enum avr_builtin_id): Add AVR_BUILTIN_INSERT_BITS.
	(avr_move_bits): Rewrite.
	(avr_fold_builtin, avr_map_metric, avr_map_decompose): New static
	functions.
	(avr_map_op_t): New typedef.
	(avr_map_op): New static variable.
	(avr_out_insert_bits, avr_has_nibble_0xf): New functions.
	(adjust_insn_length): Handle ADJUST_LEN_INSERT_BITS.
	(avr_init_builtins): Add definition for __builtin_avr_insert_bits.
	(bdesc_3arg, avr_expand_triop_builtin): New.
	(avr_expand_builtin): Use them. And handle AVR_BUILTIN_INSERT_BITS.

	(avr_revert_map, avr_swap_map, avr_id_map, avr_sig_map): Remove.
	(avr_map_hamming_byte, avr_map_hamming_nonstrict): Remove.
	(avr_map_equal_p, avr_map_sig_p): Remove.
	(avr_out_swap_bits, avr_out_revert_bits, avr_out_map_bits): Remove.
	(bdesc_2arg): Remove AVR_BUILTIN_MAP8, AVR_BUILTIN_MAP16.
	(adjust_insn_length): Remove handling for ADJUST_LEN_MAP_BITS.
	(enum avr_builtin_id): Remove AVR_BUILTIN_MAP8, AVR_BUILTIN_MAP16.
	(avr_init_builtins): Remove __builtin_avr_map8, __builtin_avr_map16.
	(avr_expand_builtin): Remove AVR_BUILTIN_MAP8, AVR_BUILTIN_MAP16.

Comments

Weddington, Eric Feb. 14, 2012, 5:32 p.m. UTC | #1 
> -----Original Message-----
> From: Georg-Johann Lay 
> Sent: Monday, February 13, 2012 9:01 AM
> To: gcc-patches@gcc.gnu.org
> Cc: Weddington, Eric; Denis Chertykov
> Subject: [Patch,AVR]: Built-in for non-contiguous port layouts
> 
> This patch set removes __builtin_avr_map8 __builtin_avr_map16
built-ins and
> implements a built-in __builtin_avr_insert_bits instead.
> 

Hi Johann,

I'm just now starting to review this patch, but I have a quick
question...

In your avr_fold_builtin function you have switch (fcode) with only one
case (and a "default" that just breaks). Are you planning to add more
cases in the near future? If not, wouldn't it be better to just make it
an 'if' statement?

Thanks,
Eric
Georg-Johann Lay Feb. 14, 2012, 5:51 p.m. UTC | #2 
Weddington, Eric wrote:
> 
>>
>> This patch set removes __builtin_avr_map8 __builtin_avr_map16
>> built-ins and implements a built-in __builtin_avr_insert_bits instead.
>>
> 
> In your avr_fold_builtin function you have switch (fcode) with only one
> case (and a "default" that just breaks). Are you planning to add more
> cases in the near future? If not, wouldn't it be better to just make it
> an 'if' statement?

The intention was to facilitate adding of other folds by means of other case
entries.  Folding swap is pretty much trivial for example.  And someone
proposed adding things like __builtin_avr_memx_flash_p resp.
__builtin_avr_memx_ram_p that could also be folded easily.

Johann
Weddington, Eric Feb. 14, 2012, 7:44 p.m. UTC | #3 
> -----Original Message-----
> From: Georg-Johann Lay 
> Sent: Tuesday, February 14, 2012 10:52 AM
> To: Weddington, Eric
> Cc: gcc-patches@gcc.gnu.org; Denis Chertykov
> Subject: Re: [Patch,AVR]: Built-in for non-contiguous port layouts
> 
> Weddington, Eric wrote:
> >
> >>
> >> This patch set removes __builtin_avr_map8 __builtin_avr_map16
> >> built-ins and implements a built-in __builtin_avr_insert_bits
instead.
> >>
> >
> > In your avr_fold_builtin function you have switch (fcode) with only
one
> > case (and a "default" that just breaks). Are you planning to add
more
> > cases in the near future? If not, wouldn't it be better to just make
it
> > an 'if' statement?
> 
> The intention was to facilitate adding of other folds by means of
other case
> entries.  Folding swap is pretty much trivial for example.  And
someone
> proposed adding things like __builtin_avr_memx_flash_p resp.
> __builtin_avr_memx_ram_p that could also be folded easily.
> 

Ok, thanks. Please commit.

Eric
diff mbox

Patch

Index: testsuite/gcc.target/avr/torture/builtin_insert_bits-1.c
===================================================================
--- testsuite/gcc.target/avr/torture/builtin_insert_bits-1.c	(revision 0)
+++ testsuite/gcc.target/avr/torture/builtin_insert_bits-1.c	(revision 0)
@@ -0,0 +1,97 @@ 
+/* { dg-do run } */
+
+#include <stdlib.h>
+
+#define MASK_F(M)                                       \
+  (0                                                    \
+   | ((0xf == (0xf & ((M) >> (4*0)))) ? (1 << 0) : 0)   \
+   | ((0xf == (0xf & ((M) >> (4*1)))) ? (1 << 1) : 0)   \
+   | ((0xf == (0xf & ((M) >> (4*2)))) ? (1 << 2) : 0)   \
+   | ((0xf == (0xf & ((M) >> (4*3)))) ? (1 << 3) : 0)   \
+   | ((0xf == (0xf & ((M) >> (4*4)))) ? (1 << 4) : 0)   \
+   | ((0xf == (0xf & ((M) >> (4*5)))) ? (1 << 5) : 0)   \
+   | ((0xf == (0xf & ((M) >> (4*6)))) ? (1 << 6) : 0)   \
+   | ((0xf == (0xf & ((M) >> (4*7)))) ? (1 << 7) : 0)   \
+   | 0)
+
+#define MASK_0_7(M)                                     \
+  (0                                                    \
+   | ((8 > (0xf & ((M) >> (4*0)))) ? (1 << 0) : 0)      \
+   | ((8 > (0xf & ((M) >> (4*1)))) ? (1 << 1) : 0)      \
+   | ((8 > (0xf & ((M) >> (4*2)))) ? (1 << 2) : 0)      \
+   | ((8 > (0xf & ((M) >> (4*3)))) ? (1 << 3) : 0)      \
+   | ((8 > (0xf & ((M) >> (4*4)))) ? (1 << 4) : 0)      \
+   | ((8 > (0xf & ((M) >> (4*5)))) ? (1 << 5) : 0)      \
+   | ((8 > (0xf & ((M) >> (4*6)))) ? (1 << 6) : 0)      \
+   | ((8 > (0xf & ((M) >> (4*7)))) ? (1 << 7) : 0)      \
+   | 0)
+
+#define INSERT_BITS(M,B,V)                                              \
+  (__extension__({                                                      \
+      unsigned char _n, _r = 0;                                         \
+      _n = 0xf & (M >> (4*0)); if (_n<8) _r |= (!!(B & (1 << _n))) << 0; \
+      _n = 0xf & (M >> (4*1)); if (_n<8) _r |= (!!(B & (1 << _n))) << 1; \
+      _n = 0xf & (M >> (4*2)); if (_n<8) _r |= (!!(B & (1 << _n))) << 2; \
+      _n = 0xf & (M >> (4*3)); if (_n<8) _r |= (!!(B & (1 << _n))) << 3; \
+      _n = 0xf & (M >> (4*4)); if (_n<8) _r |= (!!(B & (1 << _n))) << 4; \
+      _n = 0xf & (M >> (4*5)); if (_n<8) _r |= (!!(B & (1 << _n))) << 5; \
+      _n = 0xf & (M >> (4*6)); if (_n<8) _r |= (!!(B & (1 << _n))) << 6; \
+      _n = 0xf & (M >> (4*7)); if (_n<8) _r |= (!!(B & (1 << _n))) << 7; \
+      (unsigned char) ((V) & MASK_F(M)) | _r;                           \
+    }))
+
+#define MASK_USED(M) (MASK_F(M) | MASK_0_7(M))
+
+#define TEST2(M,B,V)                                    \
+  do {                                                  \
+    __asm volatile (";" #M);                            \
+    r1 = MASK_USED (M)                                  \
+      & __builtin_avr_insert_bits (M,B,V);              \
+    r2 = INSERT_BITS (M,B,V);                           \
+    if (r1 != r2)                                       \
+      abort ();                                         \
+  } while(0)
+
+#define TEST1(M,X)                                      \
+  do {                                                  \
+    TEST2 (M,X,0x00); TEST2 (M,0x00,X);                 \
+    TEST2 (M,X,0xff); TEST2 (M,0xff,X);                 \
+    TEST2 (M,X,0xaa); TEST2 (M,0xaa,X);                 \
+    TEST2 (M,X,0xcc); TEST2 (M,0xcc,X);                 \
+    TEST2 (M,X,0x96); TEST2 (M,0x96,X);                 \
+  } while(0)
+
+
+
+void test8 (void)
+{
+  unsigned char r1, r2;
+  unsigned char ib;
+
+  static const unsigned char V[] =
+    {
+      0, 0xaa, 0xcc, 0xf0, 0xff, 0x5b, 0x4d
+    };
+
+  for (ib = 0; ib < sizeof (V) / sizeof (*V); ib++)
+    {
+      unsigned char b = V[ib];
+      
+      TEST1 (0x76543210, b);
+      TEST1 (0x3210ffff, b);
+      TEST1 (0x67452301, b);
+      TEST1 (0xf0f1f2f3, b);
+      TEST1 (0xff10ff54, b);
+      TEST1 (0x01234567, b);
+      TEST1 (0xff765f32, b);
+    }
+}
+
+/****************************************************************/
+
+int main()
+{
+  test8();
+  
+  exit(0);
+}
Index: testsuite/gcc.target/avr/torture/builtin_insert_bits-2.c
===================================================================
--- testsuite/gcc.target/avr/torture/builtin_insert_bits-2.c	(revision 0)
+++ testsuite/gcc.target/avr/torture/builtin_insert_bits-2.c	(revision 0)
@@ -0,0 +1,94 @@ 
+/* { dg-do run } */
+
+#include <stdlib.h>
+
+#define MASK_F(M)                                       \
+  (0                                                    \
+   | ((0xf == (0xf & ((M) >> (4*0)))) ? (1 << 0) : 0)   \
+   | ((0xf == (0xf & ((M) >> (4*1)))) ? (1 << 1) : 0)   \
+   | ((0xf == (0xf & ((M) >> (4*2)))) ? (1 << 2) : 0)   \
+   | ((0xf == (0xf & ((M) >> (4*3)))) ? (1 << 3) : 0)   \
+   | ((0xf == (0xf & ((M) >> (4*4)))) ? (1 << 4) : 0)   \
+   | ((0xf == (0xf & ((M) >> (4*5)))) ? (1 << 5) : 0)   \
+   | ((0xf == (0xf & ((M) >> (4*6)))) ? (1 << 6) : 0)   \
+   | ((0xf == (0xf & ((M) >> (4*7)))) ? (1 << 7) : 0)   \
+   | 0)
+
+#define MASK_0_7(M)                                     \
+  (0                                                    \
+   | ((8 > (0xf & ((M) >> (4*0)))) ? (1 << 0) : 0)      \
+   | ((8 > (0xf & ((M) >> (4*1)))) ? (1 << 1) : 0)      \
+   | ((8 > (0xf & ((M) >> (4*2)))) ? (1 << 2) : 0)      \
+   | ((8 > (0xf & ((M) >> (4*3)))) ? (1 << 3) : 0)      \
+   | ((8 > (0xf & ((M) >> (4*4)))) ? (1 << 4) : 0)      \
+   | ((8 > (0xf & ((M) >> (4*5)))) ? (1 << 5) : 0)      \
+   | ((8 > (0xf & ((M) >> (4*6)))) ? (1 << 6) : 0)      \
+   | ((8 > (0xf & ((M) >> (4*7)))) ? (1 << 7) : 0)      \
+   | 0)
+
+#define INSERT_BITS(M,B,V)                                              \
+  (__extension__({                                                      \
+      unsigned char _n, _r = 0;                                         \
+      _n = 0xf & (M >> (4*0)); if (_n<8) _r |= (!!(B & (1 << _n))) << 0; \
+      _n = 0xf & (M >> (4*1)); if (_n<8) _r |= (!!(B & (1 << _n))) << 1; \
+      _n = 0xf & (M >> (4*2)); if (_n<8) _r |= (!!(B & (1 << _n))) << 2; \
+      _n = 0xf & (M >> (4*3)); if (_n<8) _r |= (!!(B & (1 << _n))) << 3; \
+      _n = 0xf & (M >> (4*4)); if (_n<8) _r |= (!!(B & (1 << _n))) << 4; \
+      _n = 0xf & (M >> (4*5)); if (_n<8) _r |= (!!(B & (1 << _n))) << 5; \
+      _n = 0xf & (M >> (4*6)); if (_n<8) _r |= (!!(B & (1 << _n))) << 6; \
+      _n = 0xf & (M >> (4*7)); if (_n<8) _r |= (!!(B & (1 << _n))) << 7; \
+      (unsigned char) ((V) & MASK_F(M)) | _r;                           \
+    }))
+
+#define MASK_USED(M) (MASK_F(M) | MASK_0_7(M))
+
+#define TEST2(M,B,V)                                    \
+  do {                                                  \
+    __asm volatile (";" #M);                            \
+    r1 = MASK_USED (M)                                  \
+      & __builtin_avr_insert_bits (M,B,V);              \
+    r2 = INSERT_BITS (M,B,V);                           \
+    if (r1 != r2)                                       \
+      abort ();                                         \
+  } while(0)
+
+void test8 (void)
+{
+  unsigned char r1, r2;
+  unsigned char ib, iv;
+
+  static const unsigned char V[] =
+    {
+      0, 0xaa, 0xcc, 0xf0, 0xff, 0x5b, 0x4d
+    };
+
+  for (ib = 0; ib < sizeof (V) / sizeof (*V); ib++)
+    {
+      unsigned char b = V[ib];
+      
+      for (iv = 0; iv < sizeof (V) / sizeof (*V); iv++)
+        {
+          unsigned char v = V[iv];
+          
+          TEST2 (0x76543210, b, v);
+          TEST2 (0xffffffff, b, v);
+          TEST2 (0x3210ffff, b, v);
+          TEST2 (0x67452301, b, v);
+          TEST2 (0xf0f1f2f3, b, v);
+          TEST2 (0xff10ff54, b, v);
+          TEST2 (0x0765f321, b, v);
+          TEST2 (0x11223344, b, v);
+          TEST2 (0x01234567, b, v);
+          TEST2 (0xff7765f3, b, v);
+        }
+    }
+}
+
+/****************************************************************/
+
+int main()
+{
+  test8();
+  
+  exit(0);
+}