https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97205
--- Comment #10 from rguenther at suse dot de <rguenther at suse dot de> --- On Wed, 28 Oct 2020, bernd.edlinger at hotmail dot de wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97205 > > --- Comment #9 from Bernd Edlinger <bernd.edlinger at hotmail dot de> --- > (In reply to rguent...@suse.de from comment #7) > > On Wed, 28 Oct 2020, bernd.edlinger at hotmail dot de wrote: > > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=97205 > > > > > > --- Comment #6 from Bernd Edlinger <bernd.edlinger at hotmail dot de> --- > > > (In reply to rguent...@suse.de from comment #3) > > > > On Tue, 27 Oct 2020, bernd.edlinger at hotmail dot de wrote: > > > > > --- a/gcc/emit-rtl.c > > > > > +++ b/gcc/emit-rtl.c > > > > > @@ -2089,7 +2089,8 @@ set_mem_attributes_minus_bitpos (rtx ref, tree > > > > > t, int > > > > > objectp, > > > > > { > > > > > gcc_assert (handled_component_p (t) > > > > > || TREE_CODE (t) == MEM_REF > > > > > - || TREE_CODE (t) == TARGET_MEM_REF); > > > > > + || TREE_CODE (t) == TARGET_MEM_REF > > > > > + || TREE_CODE (t) == SSA_NAME); > > > > > > > > Can you track down this? It's a red herring to have a MEM_EXPR > > > > that just is a SSA_NAME. > > > > > > > > > > This happens here: > > > > > > if (MEM_P (to_rtx)) > > > { > > > /* If the field is at offset zero, we could have been given > > > the > > > DECL_RTX of the parent struct. Don't munge it. */ > > > to_rtx = shallow_copy_rtx (to_rtx); > > > set_mem_attributes_minus_bitpos (to_rtx, to, 0, bitpos); > > > if (volatilep) > > > MEM_VOLATILE_P (to_rtx) = 1; > > > } > > > > > > since the patch allows the SSA_NAME to reach this block. > > > > > > > > > > > --- a/gcc/expr.c > > > > > +++ b/gcc/expr.c > > > > > @@ -5200,6 +5200,9 @@ expand_assignment (tree to, tree from, bool > > > > > nontemporal) > > > > > || (TREE_CODE (to) == MEM_REF > > > > > && (REF_REVERSE_STORAGE_ORDER (to) > > > > > || mem_ref_refers_to_non_mem_p (to))) > > > > > + || (TREE_CODE (to) == SSA_NAME > > > > > + && mode != BLKmode > > > > > + && TYPE_ALIGN (TREE_TYPE (to)) < GET_MODE_ALIGNMENT (mode)) > > > > > > > > But an SSA name is a register, esp. one with non-BLKmode. And if > > > > expanded to a stack location that stack location should better > > > > be aligned... or be BLKmode. So I think the bug is elsewhere? > > > > > > > > > > Hmm, to avoid the ICE the SSA_NAME would need a naturally > > > aligned type, maybe replace it somehow in make_ssa_name_fn ... > > > > Ah, OK - only now looked at the testcase ;) Yes, I think for > > registers we should ignore decl/type alignment and _always_ > > use the natural (mode) alignment. That is, > > > > static unsigned int > > align_local_variable (tree decl, bool really_expand) > > { > > unsigned int align; > > > > if (TREE_CODE (decl) == SSA_NAME) > > align = TYPE_ALIGN (TREE_TYPE (decl)); > > > > should probably use GET_MODE_ALIGNMENT (TYPE_MODE (TREE_TYPE (decl))) > > unless it is a BLKmode var. There's a duplicate code in > > expand_one_stack_var_1, not sure why it special-cases SSA_NAMEs there. > > > > Ah, yes, using a similar strategy as we did in r274986 > where we aligned param_decls, I would say this > completely untested patch goes in the same direction: > > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c > index f3f17d3..030d0cb 100644 > --- a/gcc/cfgexpand.c > +++ b/gcc/cfgexpand.c > @@ -366,7 +366,18 @@ align_local_variable (tree decl, bool really_expand) > unsigned int align; > > if (TREE_CODE (decl) == SSA_NAME) > - align = TYPE_ALIGN (TREE_TYPE (decl)); > + { > + tree type = TREE_TYPE (decl); > + machine_mode mode = TYPE_MODE (type); > + > + align = TYPE_ALIGN (type); > + if (mode != BLKmode > + && align < GET_MODE_ALIGNMENT (mode) > + && ((optab_handler (movmisalign_optab, mode) > + != CODE_FOR_nothing) > + || targetm.slow_unaligned_access (mode, align))) I wouldn't even restrict it this way - after all if we have an SSA_NAME its address hasn't been exposed and thus any different user requested alignment isn't exposed. Instead we can work to produce optimal code which means aligning according to the mode (that's what the RA would do when spilling the 'register'). Note that this would argue we could fix the types for SSA names upfront but there might be some complication when we have to adjust the underlying decl and that's a PARM_DECL. > + align = GET_MODE_ALIGNMENT (mode); > + } > else > { > align = LOCAL_DECL_ALIGNMENT (decl); > @@ -1022,6 +1033,17 @@ expand_one_stack_var_at (tree decl, rtx base, unsigned > base_align, > } > > set_rtl (decl, x); > + > + if (TREE_CODE (decl) == SSA_NAME > + && GET_MODE (x) != BLKmode > + && MEM_ALIGN (x) < GET_MODE_ALIGNMENT (GET_MODE (x)) > + && ((optab_handler (movmisalign_optab, GET_MODE (x)) > + != CODE_FOR_nothing) > + || targetm.slow_unaligned_access (GET_MODE (x), MEM_ALIGN (x)))) I failed to track down where we'd expand this to a possibly unaligned mem - but is this just bogus MEM_ALIGN set? Can we instead fix that somehow? > + { > + gcc_checking_assert (GET_MODE_ALIGNMENT (GET_MODE (x)) <= base_align); > + set_mem_align (x, GET_MODE_ALIGNMENT (GET_MODE (x))); > + } > } > > class stack_vars_data > @@ -1327,13 +1349,11 @@ expand_one_stack_var_1 (tree var) > { > tree type = TREE_TYPE (var); > size = tree_to_poly_uint64 (TYPE_SIZE_UNIT (type)); > - byte_align = TYPE_ALIGN_UNIT (type); > } > else > - { > - size = tree_to_poly_uint64 (DECL_SIZE_UNIT (var)); > - byte_align = align_local_variable (var, true); > - } > + size = tree_to_poly_uint64 (DECL_SIZE_UNIT (var)); > + > + byte_align = align_local_variable (var, true); > > /* We handle highly aligned variables in expand_stack_vars. */ > gcc_assert (byte_align * BITS_PER_UNIT <= MAX_SUPPORTED_STACK_ALIGNMENT); > > > > Also, when an SSA name gets a stack location, we should instead use > > the underlying decl for the MEM_EXPR, not the SSA name. > > > > At least the MEM_EXPRs from this test case don't do that. Figured that. Since we don't always have an underlying decl it's probably error-prone. > > > But isn't it possible that expand_expr_real_1 > > > returns the original unaligned decl here? > > > > > > case GIMPLE_SINGLE_RHS: > > > { > > > r = expand_expr_real (gimple_assign_rhs1 (g), target, > > > tmode, modifier, alt_rtl, > > > inner_reference_p); > > > break; > > > } > > > default: > > > gcc_unreachable (); > > > } > > > set_curr_insn_location (saved_loc); > > > if (REG_P (r) && !REG_EXPR (r)) > > > set_reg_attrs_for_decl_rtl (SSA_NAME_VAR (exp), r); > > > return r; > > My only concern left is if this code path might somehow return an > misaligned MEM_P from that gimple_assign_rhs, but probably > there is an obvious reason why that may never happen. never say never ... but unless we have a testcase we just claim so.
