This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Don't query the frontend for unsupported types
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Richard Biener <richard dot guenther at gmail dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>, Richard Sandiford <richard dot sandiford at linaro dot org>
- Date: Wed, 25 Oct 2017 15:09:52 +0200
- Subject: Re: Don't query the frontend for unsupported types
- Authentication-results: sourceware.org; auth=none
- References: <87shfhzgwc.fsf@linaro.org> <FDB20764-7B41-4630-B4C6-3DEAFCE53C58@gmail.com> <874lrw5hxk.fsf@linaro.org> <CAFiYyc35uOLn4E1qVR+4PERDL3vumapyZt3OiLGxn8MXGWANrQ@mail.gmail.com> <87zi9mptv6.fsf@linaro.org> <CAFiYyc1+Bx0SFLqM=0HWxLkMpDZXWq1fs5Jkie=DDpk1j-ZS3A@mail.gmail.com> <87k20ex2ps.fsf@linaro.org>
On Sun, Oct 1, 2017 at 6:17 PM, Richard Sandiford
<richard.sandiford@linaro.org> wrote:
> Richard Biener <richard.guenther@gmail.com> writes:
>> On Fri, Sep 22, 2017 at 6:42 PM, Richard Sandiford
>> <richard.sandiford@linaro.org> wrote:
>>> Richard Biener <richard.guenther@gmail.com> writes:
>>>> On Thu, Sep 21, 2017 at 2:56 PM, Richard Sandiford
>>>> <richard.sandiford@linaro.org> wrote:
>>>>> Richard Biener <richard.guenther@gmail.com> writes:
>>>>>> On September 20, 2017 2:36:03 PM GMT+02:00, Richard Sandiford
>>>>>> <richard.sandiford@linaro.org> wrote:
>>>>>>>When forcing a constant of mode MODE into memory, force_const_mem
>>>>>>>asks the frontend to provide the type associated with that mode.
>>>>>>>In principle type_for_mode is allowed to return null, and although
>>>>>>>one use site correctly handled that, the other didn't.
>>>>>>>
>>>>>>>I think there's agreement that it's bogus to use type_for_mode for
>>>>>>>this kind of thing, since it forces frontends to handle types that
>>>>>>>don't exist in that language. See e.g. http://gcc.gnu.org/PR46805
>>>>>>>where the Go frontend was forced to handle vector types even though
>>>>>>>Go doesn't have vector types.
>>>>>>>
>>>>>>>Also, the frontends use code like:
>>>>>>>
>>>>>>> else if (VECTOR_MODE_P (mode))
>>>>>>> {
>>>>>>> machine_mode inner_mode = GET_MODE_INNER (mode);
>>>>>>> tree inner_type = c_common_type_for_mode (inner_mode, unsignedp);
>>>>>>> if (inner_type != NULL_TREE)
>>>>>>> return build_vector_type_for_mode (inner_type, mode);
>>>>>>> }
>>>>>>>
>>>>>>>and there's no guarantee that every vector mode M used by backend
>>>>>>>rtl has an associated vector type whose TYPE_MODE is M. I think
>>>>>>>really the type_for_mode hook should only return trees that _do_ have
>>>>>>>the requested TYPE_MODE, but PR46805 linked above shows that this is
>>>>>>>likely to have too many knock-on consequences. It doesn't make sense
>>>>>>>for force_const_mem to ask about vector modes that aren't valid for
>>>>>>>vector types, so this patch handles the condition there instead.
>>>>>>>
>>>>>>>This is needed for SVE multi-register modes, which are modelled as
>>>>>>>vector modes but are not usable as vector types.
>>>>>>>
>>>>>>>Tested on aarch64-linux-gnu, x86_64-linux-gnu and
>>>>>>>powerpc64le-linus-gnu.
>>>>>>>OK to install?
>>>>>>
>>>>>> I think we should get rid of the use entirely.
>>>>>
>>>>> I first read this as not using type_for_mode at all in force_const_mem,
>>>>> which sounded like a good thing :-)
>>>>
>>>> That's what I meant ;) A mode doesn't really have a type...
>>>>
>>>> I tried it overnight on the usual
>>>>> at-least-one-target-per-CPU set and diffing the before and after
>>>>> assembly for the testsuite. And it looks like i686 relies on this
>>>>> to get an alignment of 16 rather than 4 for XFmode constants:
>>>>> GET_MODE_ALIGNMENT (XFmode) == 32 (as requested by i386-modes.def),
>>>>> but i386's CONSTANT_ALIGNMENT increases it to 128 for static constants.
>>>>
>>>> Then the issue is that CONSTANT_ALIGNMENT takes a tree and not a mode...
>>>> even worse than type_for_mode is a use of make_tree! Incidentially
>>>> ix86_constant_alignment _does_ look at the mode in the end...
>>>
>>> OK, I guess this means another target hook conversion. The patch
>>> below converts CONSTANT_ALIGNMENT with its current interface.
>>> The definition:
>>>
>>> #define CONSTANT_ALIGNMENT(EXP, ALIGN) \
>>> (TREE_CODE (EXP) == STRING_CST \
>>> && (ALIGN) < BITS_PER_WORD ? BITS_PER_WORD : (ALIGN))
>>>
>>> was very common, so the patch adds a canned definition for that,
>>> called constant_alignment_word_strings. Some ports had a variation
>>> that used a port-local FASTEST_ALIGNMENT instead of BITS_PER_WORD;
>>> the patch uses constant_alignment_word_strings if FASTEST_ALIGNMENT
>>> was always BITS_PER_WORD and a port-local hook function otherwise.
>>>
>>> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64le-linux-gnu.
>>> Also tested by comparing the testsuite assembly output on at least one
>>> target per CPU directory. I don't think this comes under Jeff's
>>> preapproval due to the constant_alignment_word_strings thing, so:
>>> OK to install?
>>
>> Ok.
>
> Thanks. A bit later than intended, but here's the follow-on to add
> the new rtx hook.
>
> Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64le-linux-gnu.
> Also tested by comparing the testsuite assembly output on at least one
> target per CPU directory. OK to install?
Ok, sorry for the delay.
Richard.
> Richard
>
>
> 2017-10-01 Richard Sandiford <richard.sandiford@linaro.org>
>
> gcc/
> * target.def (static_rtx_alignment): New hook.
> * targhooks.h (default_static_rtx_alignment): Declare.
> * targhooks.c (default_static_rtx_alignment): New function.
> * doc/tm.texi.in (TARGET_STATIC_RTX_ALIGNMENT): New hook.
> * doc/tm.texi: Regenerate.
> * varasm.c (force_const_mem): Use targetm.static_rtx_alignment
> instead of targetm.constant_alignment. Remove call to
> set_mem_attributes.
> * config/cris/cris.c (TARGET_STATIC_RTX_ALIGNMENT): Redefine.
> (cris_preferred_mininum_alignment): New function, split out from...
> (cris_constant_alignment): ...here.
> (cris_static_rtx_alignment): New function.
> * config/i386/i386.c (ix86_static_rtx_alignment): New function,
> split out from...
> (ix86_constant_alignment): ...here.
> (TARGET_STATIC_RTX_ALIGNMENT): Redefine.
> * config/mmix/mmix.c (TARGET_STATIC_RTX_ALIGNMENT): Redefine.
> (mmix_static_rtx_alignment): New function.
> * config/spu/spu.c (spu_static_rtx_alignment): New function.
> (TARGET_STATIC_RTX_ALIGNMENT): Redefine.
>
> Index: gcc/target.def
> ===================================================================
> --- gcc/target.def 2017-09-25 17:04:16.792359030 +0100
> +++ gcc/target.def 2017-10-01 17:14:18.480815538 +0100
> @@ -3336,6 +3336,15 @@ HOOK_VECTOR_END (addr_space)
> #define HOOK_PREFIX "TARGET_"
>
> DEFHOOK
> +(static_rtx_alignment,
> + "This hook returns the preferred alignment in bits for a\n\
> +statically-allocated rtx, such as a constant pool entry. @var{mode}\n\
> +is the mode of the rtx. The default implementation returns\n\
> +@samp{GET_MODE_ALIGNMENT (@var{mode})}.",
> + HOST_WIDE_INT, (machine_mode mode),
> + default_static_rtx_alignment)
> +
> +DEFHOOK
> (constant_alignment,
> "This hook returns the alignment in bits of a constant that is being\n\
> placed in memory. @var{constant} is the constant and @var{basic_align}\n\
> Index: gcc/targhooks.h
> ===================================================================
> --- gcc/targhooks.h 2017-09-25 17:04:16.793358890 +0100
> +++ gcc/targhooks.h 2017-10-01 17:14:18.481821912 +0100
> @@ -93,6 +93,7 @@ extern int default_builtin_vectorization
>
> extern tree default_builtin_reciprocal (tree);
>
> +extern HOST_WIDE_INT default_static_rtx_alignment (machine_mode);
> extern HOST_WIDE_INT default_constant_alignment (const_tree, HOST_WIDE_INT);
> extern HOST_WIDE_INT constant_alignment_word_strings (const_tree,
> HOST_WIDE_INT);
> Index: gcc/targhooks.c
> ===================================================================
> --- gcc/targhooks.c 2017-09-25 17:04:16.793358890 +0100
> +++ gcc/targhooks.c 2017-10-01 17:14:18.481821912 +0100
> @@ -1165,6 +1165,14 @@ tree default_mangle_decl_assembler_name
> return id;
> }
>
> +/* The default implementation of TARGET_STATIC_RTX_ALIGNMENT. */
> +
> +HOST_WIDE_INT
> +default_static_rtx_alignment (machine_mode mode)
> +{
> + return GET_MODE_ALIGNMENT (mode);
> +}
> +
> /* The default implementation of TARGET_CONSTANT_ALIGNMENT. */
>
> HOST_WIDE_INT
> Index: gcc/doc/tm.texi.in
> ===================================================================
> --- gcc/doc/tm.texi.in 2017-09-25 17:04:16.792359030 +0100
> +++ gcc/doc/tm.texi.in 2017-10-01 17:14:18.480815538 +0100
> @@ -1026,6 +1026,8 @@ On 32-bit ELF the largest supported sect
> @samp{(0x80000000 * 8)}, but this is not representable on 32-bit hosts.
> @end defmac
>
> +@hook TARGET_STATIC_RTX_ALIGNMENT
> +
> @defmac DATA_ALIGNMENT (@var{type}, @var{basic-align})
> If defined, a C expression to compute the alignment for a variable in
> the static store. @var{type} is the data type, and @var{basic-align} is
> Index: gcc/doc/tm.texi
> ===================================================================
> --- gcc/doc/tm.texi 2017-09-25 17:04:16.791359170 +0100
> +++ gcc/doc/tm.texi 2017-10-01 17:14:18.479809163 +0100
> @@ -1078,6 +1078,13 @@ On 32-bit ELF the largest supported sect
> @samp{(0x80000000 * 8)}, but this is not representable on 32-bit hosts.
> @end defmac
>
> +@deftypefn {Target Hook} HOST_WIDE_INT TARGET_STATIC_RTX_ALIGNMENT (machine_mode @var{mode})
> +This hook returns the preferred alignment in bits for a
> +statically-allocated rtx, such as a constant pool entry. @var{mode}
> +is the mode of the rtx. The default implementation returns
> +@samp{GET_MODE_ALIGNMENT (@var{mode})}.
> +@end deftypefn
> +
> @defmac DATA_ALIGNMENT (@var{type}, @var{basic-align})
> If defined, a C expression to compute the alignment for a variable in
> the static store. @var{type} is the data type, and @var{basic-align} is
> Index: gcc/varasm.c
> ===================================================================
> --- gcc/varasm.c 2017-09-25 17:04:16.793358890 +0100
> +++ gcc/varasm.c 2017-10-01 17:14:18.481821912 +0100
> @@ -3784,11 +3784,8 @@ force_const_mem (machine_mode mode, rtx
> *slot = desc;
>
> /* Align the location counter as required by EXP's data type. */
> - align = GET_MODE_ALIGNMENT (mode == VOIDmode ? word_mode : mode);
> -
> - tree type = lang_hooks.types.type_for_mode (mode, 0);
> - if (type != NULL_TREE)
> - align = targetm.constant_alignment (make_tree (type, x), align);
> + machine_mode align_mode = (mode == VOIDmode ? word_mode : mode);
> + align = targetm.static_rtx_alignment (align_mode);
>
> pool->offset += (align / BITS_PER_UNIT) - 1;
> pool->offset &= ~ ((align / BITS_PER_UNIT) - 1);
> @@ -3830,7 +3827,6 @@ force_const_mem (machine_mode mode, rtx
>
> /* Construct the MEM. */
> desc->mem = def = gen_const_mem (mode, symbol);
> - set_mem_attributes (def, lang_hooks.types.type_for_mode (mode, 0), 1);
> set_mem_align (def, align);
>
> /* If we're dropping a label to the constant pool, make sure we
> Index: gcc/config/cris/cris.c
> ===================================================================
> --- gcc/config/cris/cris.c 2017-09-25 17:04:16.762363228 +0100
> +++ gcc/config/cris/cris.c 2017-10-01 17:14:18.472764540 +0100
> @@ -165,6 +165,7 @@ static bool cris_function_value_regno_p
> static void cris_file_end (void);
> static unsigned int cris_hard_regno_nregs (unsigned int, machine_mode);
> static bool cris_hard_regno_mode_ok (unsigned int, machine_mode);
> +static HOST_WIDE_INT cris_static_rtx_alignment (machine_mode);
> static HOST_WIDE_INT cris_constant_alignment (const_tree, HOST_WIDE_INT);
>
> /* This is the parsed result of the "-max-stack-stackframe=" option. If
> @@ -288,6 +289,8 @@ #define TARGET_HARD_REGNO_NREGS cris_har
> #undef TARGET_HARD_REGNO_MODE_OK
> #define TARGET_HARD_REGNO_MODE_OK cris_hard_regno_mode_ok
>
> +#undef TARGET_STATIC_RTX_ALIGNMENT
> +#define TARGET_STATIC_RTX_ALIGNMENT cris_static_rtx_alignment
> #undef TARGET_CONSTANT_ALIGNMENT
> #define TARGET_CONSTANT_ALIGNMENT cris_constant_alignment
>
> @@ -4329,6 +4332,26 @@ cris_hard_regno_mode_ok (unsigned int re
> || (regno != CRIS_MOF_REGNUM && regno != CRIS_ACR_REGNUM)));
> }
>
> +/* Return the preferred minimum alignment for a static object. */
> +
> +static HOST_WIDE_INT
> +cris_preferred_mininum_alignment (void)
> +{
> + if (!TARGET_CONST_ALIGN)
> + return 8;
> + if (TARGET_ALIGN_BY_32)
> + return 32;
> + return 16;
> +}
> +
> +/* Implement TARGET_STATIC_RTX_ALIGNMENT. */
> +
> +static HOST_WIDE_INT
> +cris_static_rtx_alignment (machine_mode mode)
> +{
> + return MAX (cris_preferred_mininum_alignment (), GET_MODE_ALIGNMENT (mode));
> +}
> +
> /* Implement TARGET_CONSTANT_ALIGNMENT. Note that this hook has the
> effect of making gcc believe that ALL references to constant stuff
> (in code segment, like strings) have this alignment. That is a rather
> @@ -4339,11 +4362,7 @@ cris_hard_regno_mode_ok (unsigned int re
> static HOST_WIDE_INT
> cris_constant_alignment (const_tree, HOST_WIDE_INT basic_align)
> {
> - if (!TARGET_CONST_ALIGN)
> - return basic_align;
> - if (TARGET_ALIGN_BY_32)
> - return MAX (basic_align, 32);
> - return MAX (basic_align, 16);
> + return MAX (cris_preferred_mininum_alignment (), basic_align);
> }
>
> #if 0
> Index: gcc/config/i386/i386.c
> ===================================================================
> --- gcc/config/i386/i386.c 2017-09-25 17:04:16.768362388 +0100
> +++ gcc/config/i386/i386.c 2017-10-01 17:14:18.477796414 +0100
> @@ -31563,6 +31563,18 @@ ix86_sched_init_global (FILE *, int, int
> }
>
>
> +/* Implement TARGET_STATIC_RTX_ALIGNMENT. */
> +
> +static HOST_WIDE_INT
> +ix86_static_rtx_alignment (machine_mode mode)
> +{
> + if (mode == DFmode)
> + return 64;
> + if (ALIGN_MODE_128 (mode))
> + return MAX (128, GET_MODE_ALIGNMENT (mode));
> + return GET_MODE_ALIGNMENT (mode);
> +}
> +
> /* Implement TARGET_CONSTANT_ALIGNMENT. */
>
> static HOST_WIDE_INT
> @@ -31571,10 +31583,9 @@ ix86_constant_alignment (const_tree exp,
> if (TREE_CODE (exp) == REAL_CST || TREE_CODE (exp) == VECTOR_CST
> || TREE_CODE (exp) == INTEGER_CST)
> {
> - if (TYPE_MODE (TREE_TYPE (exp)) == DFmode && align < 64)
> - return 64;
> - else if (ALIGN_MODE_128 (TYPE_MODE (TREE_TYPE (exp))) && align < 128)
> - return 128;
> + machine_mode mode = TYPE_MODE (TREE_TYPE (exp));
> + HOST_WIDE_INT mode_align = ix86_static_rtx_alignment (mode);
> + return MAX (mode_align, align);
> }
> else if (!optimize_size && TREE_CODE (exp) == STRING_CST
> && TREE_STRING_LENGTH (exp) >= 31 && align < BITS_PER_WORD)
> @@ -53605,6 +53616,8 @@ #define TARGET_HARD_REGNO_CALL_PART_CLOB
> #undef TARGET_CAN_CHANGE_MODE_CLASS
> #define TARGET_CAN_CHANGE_MODE_CLASS ix86_can_change_mode_class
>
> +#undef TARGET_STATIC_RTX_ALIGNMENT
> +#define TARGET_STATIC_RTX_ALIGNMENT ix86_static_rtx_alignment
> #undef TARGET_CONSTANT_ALIGNMENT
> #define TARGET_CONSTANT_ALIGNMENT ix86_constant_alignment
>
> Index: gcc/config/mmix/mmix.c
> ===================================================================
> --- gcc/config/mmix/mmix.c 2017-09-25 17:04:16.774361549 +0100
> +++ gcc/config/mmix/mmix.c 2017-10-01 17:14:18.477796414 +0100
> @@ -168,6 +168,7 @@ static void mmix_print_operand (FILE *,
> static void mmix_print_operand_address (FILE *, machine_mode, rtx);
> static bool mmix_print_operand_punct_valid_p (unsigned char);
> static void mmix_conditional_register_usage (void);
> +static HOST_WIDE_INT mmix_static_rtx_alignment (machine_mode);
> static HOST_WIDE_INT mmix_constant_alignment (const_tree, HOST_WIDE_INT);
>
> /* Target structure macros. Listed by node. See `Using and Porting GCC'
> @@ -283,6 +284,8 @@ #define TARGET_TRAMPOLINE_INIT mmix_tram
> #undef TARGET_OPTION_OVERRIDE
> #define TARGET_OPTION_OVERRIDE mmix_option_override
>
> +#undef TARGET_STATIC_RTX_ALIGNMENT
> +#define TARGET_STATIC_RTX_ALIGNMENT mmix_static_rtx_alignment
> #undef TARGET_CONSTANT_ALIGNMENT
> #define TARGET_CONSTANT_ALIGNMENT mmix_constant_alignment
>
> @@ -338,6 +341,14 @@ mmix_data_alignment (tree type ATTRIBUTE
> return basic_align;
> }
>
> +/* Implement TARGET_STATIC_RTX_ALIGNMENT. */
> +
> +static HOST_WIDE_INT
> +mmix_static_rtx_alignment (machine_mode mode)
> +{
> + return MAX (GET_MODE_ALIGNMENT (mode), 32);
> +}
> +
> /* Implement tARGET_CONSTANT_ALIGNMENT. */
>
> static HOST_WIDE_INT
> Index: gcc/config/spu/spu.c
> ===================================================================
> --- gcc/config/spu/spu.c 2017-09-25 17:04:16.787359730 +0100
> +++ gcc/config/spu/spu.c 2017-10-01 17:14:18.478802788 +0100
> @@ -7194,6 +7194,18 @@ spu_truly_noop_truncation (unsigned int
> return inprec <= 32 && outprec <= inprec;
> }
>
> +/* Implement TARGET_STATIC_RTX_ALIGNMENT.
> +
> + Make all static objects 16-byte aligned. This allows us to assume
> + they are also padded to 16 bytes, which means we can use a single
> + load or store instruction to access them. */
> +
> +static HOST_WIDE_INT
> +spu_static_rtx_alignment (machine_mode mode)
> +{
> + return MAX (GET_MODE_ALIGNMENT (mode), 128);
> +}
> +
> /* Implement TARGET_CONSTANT_ALIGNMENT.
>
> Make all static objects 16-byte aligned. This allows us to assume
> @@ -7445,6 +7457,8 @@ #define TARGET_CAN_CHANGE_MODE_CLASS spu
> #undef TARGET_TRULY_NOOP_TRUNCATION
> #define TARGET_TRULY_NOOP_TRUNCATION spu_truly_noop_truncation
>
> +#undef TARGET_STATIC_RTX_ALIGNMENT
> +#define TARGET_STATIC_RTX_ALIGNMENT spu_static_rtx_alignment
> #undef TARGET_CONSTANT_ALIGNMENT
> #define TARGET_CONSTANT_ALIGNMENT spu_constant_alignment
>