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