On Thu, Sep 21, 2017 at 2:56 PM, Richard Sandiford <richard.sandif...@linaro.org> wrote: > Richard Biener <richard.guent...@gmail.com> writes: >> On September 20, 2017 2:36:03 PM GMT+02:00, Richard Sandiford >> <richard.sandif...@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... > 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 <richard.sandif...@linaro.org> > Alan Hayward <alan.hayw...@arm.com> > David Sherwood <david.sherw...@arm.com> > > 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.c 2017-09-21 11:17:14.726201207 +0100 > +++ gcc/varasm.c 2017-09-21 13:54:22.209159021 +0100 > @@ -3785,10 +3785,17 @@ force_const_mem (machine_mode mode, rtx > desc = ggc_alloc<constant_descriptor_rtx> (); > *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
