Re: Don't query the frontend for unsupported types

[Richard Biener]

On Sun, Oct 1, 2017 at 6:17 PM, Richard Sandiford
 wrote:
> Richard Biener  writes:
>> On Fri, Sep 22, 2017 at 6:42 PM, Richard Sandiford
>>  wrote:
>>> Richard Biener  writes:
 On Thu, Sep 21, 2017 at 2:56 PM, Richard Sandiford
  wrote:
> Richard Biener  writes:
>> On September 20, 2017 2:36:03 PM GMT+02:00, Richard Sandiford
>>  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  
>
> 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 

Re: Don't query the frontend for unsupported types

[Richard Sandiford]

Ping.

Richard Sandiford  writes:
> Richard Biener  writes:
>> On Fri, Sep 22, 2017 at 6:42 PM, Richard Sandiford
>>  wrote:
>>> Richard Biener  writes:
 On Thu, Sep 21, 2017 at 2:56 PM, Richard Sandiford
  wrote:
> Richard Biener  writes:
>> On September 20, 2017 2:36:03 PM GMT+02:00, Richard Sandiford
>>  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?
>
> Richard
>
>
> 2017-10-01  Richard Sandiford  
>
> 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.  

Re: Don't query the frontend for unsupported types

[Richard Sandiford]

Richard Biener  writes:
> On Fri, Sep 22, 2017 at 6:42 PM, Richard Sandiford
>  wrote:
>> Richard Biener  writes:
>>> On Thu, Sep 21, 2017 at 2:56 PM, Richard Sandiford
>>>  wrote:
 Richard Biener  writes:
> On September 20, 2017 2:36:03 PM GMT+02:00, Richard Sandiford
>  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?

Richard


2017-10-01  Richard Sandiford  

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, 

Re: Don't query the frontend for unsupported types

[Richard Biener]

On Fri, Sep 22, 2017 at 6:42 PM, Richard Sandiford
 wrote:
> Richard Biener  writes:
>> On Thu, Sep 21, 2017 at 2:56 PM, Richard Sandiford
>>  wrote:
>>> Richard Biener  writes:
 On September 20, 2017 2:36:03 PM GMT+02:00, Richard Sandiford
  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,
Richard.

> If so, then I'll follow up with a separate hook for rtl modes, which
> varasm, default_constant_alignment and constant_alignment_word_strings
> can all use.
>
> Thanks,
> Richard
>
>
> 2017-09-22  Richard Sandiford  
>
> gcc/
> * target.def (constant_alignment): New hook.
> * defaults.h (CONSTANT_ALIGNMENT): Delete.
> * doc/tm.texi.in (CONSTANT_ALIGNMENT): Replace with...
> (TARGET_CONSTANT_ALIGNMENT): ...this new hook.
> * doc/tm.texi: Regenerate.
> * targhooks.h (default_constant_alignment): Declare.
> (constant_alignment_word_strings): Likewise.
> * targhooks.c (default_constant_alignment): New function.
> (constant_alignment_word_strings): Likewise.
> * builtins.c (get_object_alignment_2): Use targetm.constant_alignment
> instead of CONSTANT_ALIGNMENT.
> * varasm.c (align_variable, get_variable_align, build_constant_desc)
> (force_const_mem): Likewise.
> * config/aarch64/aarch64.h 

Re: Don't query the frontend for unsupported types

[Richard Sandiford]

Richard Biener  writes:
> On Thu, Sep 21, 2017 at 2:56 PM, Richard Sandiford
>  wrote:
>> Richard Biener  writes:
>>> On September 20, 2017 2:36:03 PM GMT+02:00, Richard Sandiford
>>>  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?

If so, then I'll follow up with a separate hook for rtl modes, which
varasm, default_constant_alignment and constant_alignment_word_strings
can all use.

Thanks,
Richard


2017-09-22  Richard Sandiford  

gcc/
* target.def (constant_alignment): New hook.
* defaults.h (CONSTANT_ALIGNMENT): Delete.
* doc/tm.texi.in (CONSTANT_ALIGNMENT): Replace with...
(TARGET_CONSTANT_ALIGNMENT): ...this new hook.
* doc/tm.texi: Regenerate.
* targhooks.h (default_constant_alignment): Declare.
(constant_alignment_word_strings): Likewise.
* targhooks.c (default_constant_alignment): New function.
(constant_alignment_word_strings): Likewise.
* builtins.c (get_object_alignment_2): Use targetm.constant_alignment
instead of CONSTANT_ALIGNMENT.
* varasm.c (align_variable, get_variable_align, build_constant_desc)
(force_const_mem): Likewise.
* config/aarch64/aarch64.h (CONSTANT_ALIGNMENT): Delete.
* config/aarch64/aarch64.c (aarch64_constant_alignment): New function.
(aarch64_classify_address): Call it instead of CONSTANT_ALIGNMENT.
(TARGET_CONSTANT_ALIGNMENT): Redefine.
* config/alpha/alpha.h 

Re: Don't query the frontend for unsupported types

[Richard Biener]

On Thu, Sep 21, 2017 at 2:56 PM, Richard Sandiford
 wrote:
> Richard Biener  writes:
>> On September 20, 2017 2:36:03 PM GMT+02:00, Richard Sandiford
>>  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...

> But now I wonder if you meant we should just get rid of:
>
>   set_mem_attributes (def, lang_hooks.types.type_for_mode (mode, 0), 1);
>
> and keep the other call to type_for_mode, as below.

Well, that one is easy to remove indeed.  I think assigning an alias-set
based on the mode of a constant is bogus anyway.

Richard.

> Thanks,
> Richard
>
>
> 2017-09-21  Richard Sandiford  
> Alan Hayward  
> David Sherwood  
>
> gcc/
> * varasm.c (force_const_mem): Don't ask the front end about
> vector modes that are not supported as vector types by the target.
> Remove call to set_mem_attributes.
>
> Index: gcc/varasm.c
> ===
> --- gcc/varasm.c2017-09-21 11:17:14.726201207 +0100
> +++ gcc/varasm.c2017-09-21 13:54:22.209159021 +0100
> @@ -3785,10 +3785,17 @@ force_const_mem (machine_mode mode, rtx
>desc = ggc_alloc ();
>*slot = desc;
>
> +  tree type = NULL_TREE;
> +  if (mode != VOIDmode
> +  /* Don't ask the frontend about vector modes if there cannot be a
> +VECTOR_TYPE whose TYPE_MODE is MODE.  */
> +  && (!VECTOR_MODE_P (mode)
> + || targetm.vector_mode_supported_p (mode)))
> +type = lang_hooks.types.type_for_mode (mode, 0);
> +
>/* 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 = CONSTANT_ALIGNMENT (make_tree (type, x), align);
>
> @@ -3832,7 +3839,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

Re: Don't query the frontend for unsupported types

[Richard Sandiford]

Richard Biener  writes:
> On September 20, 2017 2:36:03 PM GMT+02:00, Richard Sandiford
>  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 :-)  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.

But now I wonder if you meant we should just get rid of:

  set_mem_attributes (def, lang_hooks.types.type_for_mode (mode, 0), 1);

and keep the other call to type_for_mode, as below.

Thanks,
Richard


2017-09-21  Richard Sandiford  
Alan Hayward  
David Sherwood  

gcc/
* varasm.c (force_const_mem): Don't ask the front end about
vector modes that are not supported as vector types by the target.
Remove call to set_mem_attributes.

Index: gcc/varasm.c
===
--- gcc/varasm.c2017-09-21 11:17:14.726201207 +0100
+++ gcc/varasm.c2017-09-21 13:54:22.209159021 +0100
@@ -3785,10 +3785,17 @@ force_const_mem (machine_mode mode, rtx
   desc = ggc_alloc ();
   *slot = desc;
 
+  tree type = NULL_TREE;
+  if (mode != VOIDmode
+  /* Don't ask the frontend about vector modes if there cannot be a
+VECTOR_TYPE whose TYPE_MODE is MODE.  */
+  && (!VECTOR_MODE_P (mode)
+ || targetm.vector_mode_supported_p (mode)))
+type = lang_hooks.types.type_for_mode (mode, 0);
+
   /* 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 = CONSTANT_ALIGNMENT (make_tree (type, x), align);
 
@@ -3832,7 +3839,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

Re: Don't query the frontend for unsupported types

[Richard Biener]

On September 20, 2017 2:36:03 PM GMT+02:00, Richard Sandiford 
 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. 

Richard. 

>Richard
>
>
>2017-09-20  Richard Sandiford  
>   Alan Hayward  
>   David Sherwood  
>
>gcc/
>   * varasm.c (force_const_mem): Don't ask the front end about
>   vector modes that are not supported as vector types by the target.
>
>Index: gcc/varasm.c
>===
>--- gcc/varasm.c   2017-09-12 14:28:56.402824780 +0100
>+++ gcc/varasm.c   2017-09-20 13:33:15.942547232 +0100
>@@ -3785,10 +3785,17 @@ force_const_mem (machine_mode mode, rtx
>   desc = ggc_alloc ();
>   *slot = desc;
> 
>+  tree type = NULL_TREE;
>+  if (mode != VOIDmode
>+  /* Don't ask the frontend about vector modes if there cannot be
>a
>+   VECTOR_TYPE whose TYPE_MODE is MODE.  */
>+  && (!VECTOR_MODE_P (mode)
>+|| targetm.vector_mode_supported_p (mode)))
>+type = lang_hooks.types.type_for_mode (mode, 0);
>+
>   /* 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 = CONSTANT_ALIGNMENT (make_tree (type, x), align);
> 
>@@ -3832,7 +3839,8 @@ 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);
>+  if (type)
>+set_mem_attributes (def, type, 1);
>   set_mem_align (def, align);
> 
>   /* If we're dropping a label to the constant pool, make sure we

Don't query the frontend for unsupported types

[Richard Sandiford]

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?

Richard


2017-09-20  Richard Sandiford  
Alan Hayward  
David Sherwood  

gcc/
* varasm.c (force_const_mem): Don't ask the front end about
vector modes that are not supported as vector types by the target.

Index: gcc/varasm.c
===
--- gcc/varasm.c2017-09-12 14:28:56.402824780 +0100
+++ gcc/varasm.c2017-09-20 13:33:15.942547232 +0100
@@ -3785,10 +3785,17 @@ force_const_mem (machine_mode mode, rtx
   desc = ggc_alloc ();
   *slot = desc;
 
+  tree type = NULL_TREE;
+  if (mode != VOIDmode
+  /* Don't ask the frontend about vector modes if there cannot be a
+VECTOR_TYPE whose TYPE_MODE is MODE.  */
+  && (!VECTOR_MODE_P (mode)
+ || targetm.vector_mode_supported_p (mode)))
+type = lang_hooks.types.type_for_mode (mode, 0);
+
   /* 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 = CONSTANT_ALIGNMENT (make_tree (type, x), align);
 
@@ -3832,7 +3839,8 @@ 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);
+  if (type)
+set_mem_attributes (def, type, 1);
   set_mem_align (def, align);
 
   /* If we're dropping a label to the constant pool, make sure we