| Message ID | 635e3240-18d7-648b-27f7-ec793eab382a@mittosystems.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PR87691: transparent_union attribute does not work with MODE_PARTIAL_INT | expand |
On 10/23/18 1:49 PM, Jozef Lawrynowicz wrote: > msp430-elf uses the partial int type __int20 for pointers in the large memory > > model. __int20 has PSImode, with bitsize of 20. > > A few DejaGNU tests fail when built with -mlarge for msp430-elf, when > transparent unions are used containing pointers. > These are: > - gcc.c-torture/compile/pr34885.c > - gcc.dg/transparent-union-{1,2,3,4,5}.c > > The issue is that the union is considered to have size of 32 bits (the > in-memory size of __int20), so unless mode_for_size as called by > compute_record_mode (both in stor-layout.c) is explicitly told to look for a > > mode of class MODE_PARTIAL_INT, then a size of 32 will always return MODE_INT. > > In this case, the union will have TYPE_MODE of SImode, but its field is > PSImode, so transparent_union has no effect. > > The attached patch fixes the issue by allowing the TYPE_MODE of a union to be > > set to the DECL_MODE of the widest field, if the mode is of class > MODE_PARTIAL_INT and the union would be passed by reference. > > Some target ABIs mandate that unions be passed in integer registers, so to > avoid any potential ABI violations, the mode of the union is only changed if > > it would be passed by reference. > > Successfully bootstrapped and regstested trunk for x86_64-pc-linux-gnu, and > msp430-elf with -mlarge. For msp430-elf with -mlarge, the above DejaGNU tests > > are also fixed. > > Ok for trunk? > > > 0001-Allow-union-TYPE_MODE-to-be-set-to-the-mode-of-the-w.patch > > From cc1ccfcc0d8adf7b0e1ca95a47a8a8e7e12fc99c Mon Sep 17 00:00:00 2001 > From: Jozef Lawrynowicz <jozef.l@mittosystems.com> > Date: Mon, 22 Oct 2018 21:02:10 +0100 > Subject: [PATCH] Allow union TYPE_MODE to be set to the mode of the widest > element if the union would be passed by reference > > 2018-10-23 Jozef Lawrynowicz <jozef.l@mittosystems.com> > > PR c/87691 > * gcc/stor-layout.c (compute_record_mode): Set TYPE_MODE of UNION_TYPE > to the mode of the widest field iff the widest field has mode class > MODE_INT, or MODE_PARTIAL_INT and the union would be passed by > reference. > * gcc/testsuite/gcc.target/msp430/pr87691.c: New test. OK. SOrry for the delay. jeff
On Tue, Oct 23, 2018 at 08:49:26PM +0100, Jozef Lawrynowicz wrote: > msp430-elf uses the partial int type __int20 for pointers in the large memory > model. __int20 has PSImode, with bitsize of 20. > > A few DejaGNU tests fail when built with -mlarge for msp430-elf, when > transparent unions are used containing pointers. > These are: > - gcc.c-torture/compile/pr34885.c > - gcc.dg/transparent-union-{1,2,3,4,5}.c > > The issue is that the union is considered to have size of 32 bits (the > in-memory size of __int20), so unless mode_for_size as called by > compute_record_mode (both in stor-layout.c) is explicitly told to look for a > mode of class MODE_PARTIAL_INT, then a size of 32 will always return MODE_INT. > In this case, the union will have TYPE_MODE of SImode, but its field is > PSImode, so transparent_union has no effect. > > The attached patch fixes the issue by allowing the TYPE_MODE of a union to be > set to the DECL_MODE of the widest field, if the mode is of class > MODE_PARTIAL_INT and the union would be passed by reference. > > Some target ABIs mandate that unions be passed in integer registers, so to > avoid any potential ABI violations, the mode of the union is only changed if > it would be passed by reference. > > Successfully bootstrapped and regstested trunk for x86_64-pc-linux-gnu, and > msp430-elf with -mlarge. For msp430-elf with -mlarge, the above DejaGNU tests > are also fixed. > > Ok for trunk? > > From cc1ccfcc0d8adf7b0e1ca95a47a8a8e7e12fc99c Mon Sep 17 00:00:00 2001 > From: Jozef Lawrynowicz <jozef.l@mittosystems.com> > Date: Mon, 22 Oct 2018 21:02:10 +0100 > Subject: [PATCH] Allow union TYPE_MODE to be set to the mode of the widest > element if the union would be passed by reference > > 2018-10-23 Jozef Lawrynowicz <jozef.l@mittosystems.com> > > PR c/87691 > * gcc/stor-layout.c (compute_record_mode): Set TYPE_MODE of UNION_TYPE > to the mode of the widest field iff the widest field has mode class > MODE_INT, or MODE_PARTIAL_INT and the union would be passed by > reference. > * gcc/testsuite/gcc.target/msp430/pr87691.c: New test. I'll just point out that you should drop the gcc/ and gcc/testsuite/ prefixes; the first entry will go to gcc/ChangeLog while the pr87691.c one to gcc/testsuite/ChangeLog. Marek
From cc1ccfcc0d8adf7b0e1ca95a47a8a8e7e12fc99c Mon Sep 17 00:00:00 2001 From: Jozef Lawrynowicz <jozef.l@mittosystems.com> Date: Mon, 22 Oct 2018 21:02:10 +0100 Subject: [PATCH] Allow union TYPE_MODE to be set to the mode of the widest element if the union would be passed by reference 2018-10-23 Jozef Lawrynowicz <jozef.l@mittosystems.com> PR c/87691 * gcc/stor-layout.c (compute_record_mode): Set TYPE_MODE of UNION_TYPE to the mode of the widest field iff the widest field has mode class MODE_INT, or MODE_PARTIAL_INT and the union would be passed by reference. * gcc/testsuite/gcc.target/msp430/pr87691.c: New test. --- gcc/stor-layout.c | 21 +++++++++++++--- gcc/testsuite/gcc.target/msp430/pr87691.c | 41 +++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 4 deletions(-) create mode 100644 gcc/testsuite/gcc.target/msp430/pr87691.c diff --git a/gcc/stor-layout.c b/gcc/stor-layout.c index 58a3aa3..c4f5f83 100644 --- a/gcc/stor-layout.c +++ b/gcc/stor-layout.c @@ -1834,7 +1834,13 @@ compute_record_mode (tree type) /* If this field is the whole struct, remember its mode so that, say, we can put a double in a class into a DF register instead of forcing it to live in the stack. */ - if (simple_cst_equal (TYPE_SIZE (type), DECL_SIZE (field))) + if (simple_cst_equal (TYPE_SIZE (type), DECL_SIZE (field)) + /* Partial int types (e.g. __int20) may have TYPE_SIZE equal to + wider types (e.g. int32), despite precision being less. Ensure + that the TYPE_MODE of the struct does not get set to the partial + int mode if there is a wider type also in the struct. */ + && known_gt (GET_MODE_PRECISION (DECL_MODE (field)), + GET_MODE_PRECISION (mode))) mode = DECL_MODE (field); /* With some targets, it is sub-optimal to access an aligned @@ -1844,10 +1850,17 @@ compute_record_mode (tree type) } /* If we only have one real field; use its mode if that mode's size - matches the type's size. This only applies to RECORD_TYPE. This - does not apply to unions. */ + matches the type's size. This generally only applies to RECORD_TYPE. + For UNION_TYPE, if the widest field is MODE_INT then use that mode. + If the widest field is MODE_PARTIAL_INT, and the union will be passed + by reference, then use that mode. */ poly_uint64 type_size; - if (TREE_CODE (type) == RECORD_TYPE + if ((TREE_CODE (type) == RECORD_TYPE + || (TREE_CODE (type) == UNION_TYPE + && (GET_MODE_CLASS (mode) == MODE_INT + || (GET_MODE_CLASS (mode) == MODE_PARTIAL_INT + && targetm.calls.pass_by_reference (pack_cumulative_args (0), + mode, type, 0))))) && mode != VOIDmode && poly_int_tree_p (TYPE_SIZE (type), &type_size) && known_eq (GET_MODE_BITSIZE (mode), type_size)) diff --git a/gcc/testsuite/gcc.target/msp430/pr87691.c b/gcc/testsuite/gcc.target/msp430/pr87691.c new file mode 100644 index 0000000..c00425d --- /dev/null +++ b/gcc/testsuite/gcc.target/msp430/pr87691.c @@ -0,0 +1,41 @@ +/* PR 87691 - Test that a union containing __int20 and a float is not treated as + 20-bits in size. */ + +/* { dg-do compile } */ +/* { dg-skip-if "no __int20 for mcpu=msp430" { *-*-* } { "-mcpu=msp430" } { "" } } */ +/* { dg-final { scan-assembler-not "MOVX.A" } } */ + +/* To move a 20-bit value from memory (using indexed or indirect register + mode), onto the stack (also addressed using indexed or indirect register + mode), MOVX.A must be used. MOVA does not support these addressing modes. + Therefore, to check that the union is not manipulated as a 20-bit type, + test that no MOVX.A instructions are present in the assembly. + + MOVA is used to fill/spill u.i, but if the union is treated as 20 bits in + size, MOVX.A would be used. No other __int20 operations are present + in the source, so there will be no valid uses of MOVX.A in the resulting + assembly. */ + +union U1 +{ + float f; + __int20 i; +}; + +union U2 +{ + __int20 i; + float f; +}; + +float foo1 (union U1 u) +{ + u.i += 42; + return u.f; +} + +float foo2 (union U2 u) +{ + u.i += 42; + return u.f; +} -- 2.7.4
msp430-elf uses the partial int type __int20 for pointers in the large memory model. __int20 has PSImode, with bitsize of 20. A few DejaGNU tests fail when built with -mlarge for msp430-elf, when transparent unions are used containing pointers. These are: - gcc.c-torture/compile/pr34885.c - gcc.dg/transparent-union-{1,2,3,4,5}.c The issue is that the union is considered to have size of 32 bits (the in-memory size of __int20), so unless mode_for_size as called by compute_record_mode (both in stor-layout.c) is explicitly told to look for a mode of class MODE_PARTIAL_INT, then a size of 32 will always return MODE_INT. In this case, the union will have TYPE_MODE of SImode, but its field is PSImode, so transparent_union has no effect. The attached patch fixes the issue by allowing the TYPE_MODE of a union to be set to the DECL_MODE of the widest field, if the mode is of class MODE_PARTIAL_INT and the union would be passed by reference. Some target ABIs mandate that unions be passed in integer registers, so to avoid any potential ABI violations, the mode of the union is only changed if it would be passed by reference. Successfully bootstrapped and regstested trunk for x86_64-pc-linux-gnu, and msp430-elf with -mlarge. For msp430-elf with -mlarge, the above DejaGNU tests are also fixed. Ok for trunk?