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))) + 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)))) + { + 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. > > 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.
