With 4.8 -fno-tree-vectorize prevents the segfault

It works on trunk with -O3, but fails if -ftree-vectorize is added.

Started with r190492.

Reduced testcase for -O3:
struct A { float a[4]; };
struct B { int b; A a; };

__attribute__((noinline, noclone)) void
bar (A &a)
{
  if (a.a[0] != 36.0f || a.a[1] != 42.0f || a.a[2] != 48.0f || a.a[3] != 54.0f)
    __builtin_abort ();
}

__attribute__((noinline, noclone)) void
foo (A &a)
{
  int i;
  A c = a;
  for (i = 0; i < 4; i++)
    c.a[i] *= 6.0f;
  a = c;
  bar (a);
}

int
main ()
{
  B b = { 5, { 6, 7, 8, 9 } };
  foo (b.a);
}

target issue then?

Looks like it, yes.
In *.jump we still have (IMHO correct):
(insn 2 4 3 2 (set (reg/v/f:DI 89 [ a ])
        (reg:DI 5 di [ a ])) pr59163-2.C:13 85 {*movdi_internal}
     (nil))
(note 3 2 6 2 NOTE_INSN_FUNCTION_BEG)
(insn 6 3 7 2 (set (reg:TI 90)
        (mem:TI (reg/v/f:DI 89 [ a ]) [3 MEM[(const struct A &)a_4(D)]+0 S16 A32])) pr59163-2.C:15 84 {*movti_internal}
     (nil))
(insn 7 6 8 2 (set (mem/c:TI (plus:DI (reg/f:DI 20 frame)
                (const_int -16 [0xfffffffffffffff0])) [3 c+0 S16 A128])
        (reg:TI 90)) pr59163-2.C:15 84 {*movti_internal}
     (nil))
...
(insn 9 8 10 2 (set (reg:V4SF 91 [ vect__7.10 ])
        (mult:V4SF (reg:V4SF 92)
            (mem/c:V4SF (plus:DI (reg/f:DI 20 frame)
                    (const_int -16 [0xfffffffffffffff0])) [2 MEM[(float *)&c]+0 S16 A128]))) pr59163-2.C:17 1269 {*mulv4sf3}
     (nil))

movti_internal handles unaligned loads properly.
Then *.dse1 transforms this into:
(insn 6 3 18 2 (set (reg:TI 90 [ MEM[(const struct A &)a_4(D)] ])
        (mem:TI (reg/v/f:DI 89 [ a ]) [3 MEM[(const struct A &)a_4(D)]+0 S16 A32])) pr59163-2.C:15 84 {*movti_internal}
     (nil))
(insn 18 6 8 2 (set (reg:V4SF 94)
        (subreg:V4SF (reg:TI 90 [ MEM[(const struct A &)a_4(D)] ]) 0)) pr59163-2.C:15 -1
     (expr_list:REG_DEAD (reg:TI 90 [ MEM[(const struct A &)a_4(D)] ])
        (nil)))
...
(insn 9 8 19 2 (set (reg:V4SF 91 [ vect__7.10 ])
        (mult:V4SF (reg:V4SF 92)
            (reg:V4SF 94))) pr59163-2.C:17 1269 {*mulv4sf3}
     (expr_list:REG_DEAD (reg:V4SF 94)
        (expr_list:REG_DEAD (reg:V4SF 92)
            (nil))))
which also looks ok to me.  But then combine combines it into:
(insn 9 8 19 2 (set (reg:V4SF 91 [ vect__7.10 ])
        (mult:V4SF (reg:V4SF 92)
            (mem:V4SF (reg/v/f:DI 89 [ a ]) [3 MEM[(const struct A &)a_4(D)]+0 S16 A32]))) pr59163-2.C:17 1269 {*mulv4sf3}
     (expr_list:REG_DEAD (reg:V4SF 92)
        (nil)))
which is wrong (for pre-AVX), because mulv4sf3 can't accept unaligned memory.
Likely the SSEx pre-AVX predicates assume that an unaligned vector load will be done using UNSPEC and thus not really mergeable here, and don't count with the fact that the load could be done using integral mode and just subreged into vector mode.  Perhaps we need new predicates for this that would fail for exactly this situation (disallow unaligned scalar load subregged into vector mode for pre-AVX) and use them everywhere where SSE? doesn't accept unaligned loads?

I think that we should disallow tie of TImode with 128bit vector modes due to different alignment requirements. Integer register pairs can load unaligned TImode without problems, while unaligned TImode will crash SSE insns.

Following patch fixes the problem:

Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c  (revision 205509)
+++ config/i386/i386.c  (working copy)
@@ -35237,6 +35237,7 @@ ix86_modes_tieable_p (enum machine_mode mode1, enu
     return (GET_MODE_SIZE (mode1) == 32
            && ix86_hard_regno_mode_ok (FIRST_SSE_REG, mode1));
   if (GET_MODE_SIZE (mode2) == 16
+      && !(mode1 == TImode || mode2 == TImode)
       && ix86_hard_regno_mode_ok (FIRST_SSE_REG, mode2))
     return (GET_MODE_SIZE (mode1) == 16
            && ix86_hard_regno_mode_ok (FIRST_SSE_REG, mode1));

However, it needs some refinements for AVX.

Maybe even better idea is to use ix86_legitimate_combined_insn and reject combinations that would result in unaligned operands of all but vector move instructions.

Created attachment 31331 [details]
gcc49-pr59163.patch

So like this?  Untested...

(In reply to Jakub Jelinek from comment #8)
> Created attachment 31331 [details]
> gcc49-pr59163.patch
> 
> So like this?  Untested...

Yes, but I think that we can also allow simple vector loads and stores - they emit movu insn when unaligned operand is detected.

For stores I think the patch already allows that, that is the
  if (GET_CODE (*x) == SET && &SET_DEST (*x) == data)
    return 1;
in there (the reason why I've added it was that for the misaligned store insns
with UNSPEC_STOREU the misaligned MEM is in SET_DEST and SET_SRC just contains some rtl with UNSPEC_STOREU embedded somewhere in it.
So, would you like:
  if (GET_CODE (*x) == SET && (&SET_DEST (*x) == data || &SET_SRC (*x) == data))
    return 1;
?  That would IMHO handle simple loads from misaligned MEM too.

(In reply to Jakub Jelinek from comment #10)
> For stores I think the patch already allows that, that is the
>   if (GET_CODE (*x) == SET && &SET_DEST (*x) == data)
>     return 1;
> in there (the reason why I've added it was that for the misaligned store
> insns
> with UNSPEC_STOREU the misaligned MEM is in SET_DEST and SET_SRC just
> contains some rtl with UNSPEC_STOREU embedded somewhere in it.
> So, would you like:
>   if (GET_CODE (*x) == SET && (&SET_DEST (*x) == data || &SET_SRC (*x) ==
> data))
>     return 1;
> ?  That would IMHO handle simple loads from misaligned MEM too.

Yes. Perhaps also special case sse4.2 string instructions (they can operate on unaligned data, too), and the patch covers everything.

Created attachment 31332 [details]
gcc49-pr59163.patch

So like this?

(In reply to Jakub Jelinek from comment #12)
> Created attachment 31332 [details]
> gcc49-pr59163.patch
> 
> So like this?

Yes, with adjusted comment in ix86_legitimate_combined_insn.

IIRC, unaligned moves won't be propagated during or after reload, so it looks to me that the approach is correct.

(In reply to Uroš Bizjak from comment #13)
> (In reply to Jakub Jelinek from comment #12)
> > Created attachment 31332 [details]
> > gcc49-pr59163.patch
> > 
> > So like this?
> 
> Yes, with adjusted comment in ix86_legitimate_combined_insn.
> 
> IIRC, unaligned moves won't be propagated during or after reload, so it
> looks to me that the approach is correct.

Running the testsuite with your patch applied exposed a minor problem:

FAIL: gcc.target/i386/sse-1.c scan-assembler-not movaps

movlps/movhps and movlpd/movhpd can also handle unaligned operands (please see ix86_expand_vector_move_misalign). We should simply tag instructions that operate on unaligned operands (attribute type = ssemovu) and check type attribute instead.

The proposed approach would mean to change all scheduler and attribute calculation checks from "ssemov" to "ssemov,ssemovu", but this would be a simple mechanical change.

(In reply to Uroš Bizjak from comment #14)
> (In reply to Uroš Bizjak from comment #13)
> > (In reply to Jakub Jelinek from comment #12)
> > > Created attachment 31332 [details]
> > > gcc49-pr59163.patch
> > > 
> > > So like this?
> > 
> > Yes, with adjusted comment in ix86_legitimate_combined_insn.
> > 
> > IIRC, unaligned moves won't be propagated during or after reload, so it
> > looks to me that the approach is correct.
> 
> Running the testsuite with your patch applied exposed a minor problem:
> 
> FAIL: gcc.target/i386/sse-1.c scan-assembler-not movaps
> 
> movlps/movhps and movlpd/movhpd can also handle unaligned operands (please
> see ix86_expand_vector_move_misalign). We should simply tag instructions
> that operate on unaligned operands (attribute type = ssemovu) and check type
> attribute instead.
> 
> The proposed approach would mean to change all scheduler and attribute
> calculation checks from "ssemov" to "ssemov,ssemovu", but this would be a
> simple mechanical change.

Yeah, I've noticed that too, plus the dumps I've used to note what instructions have been rejected by the patch show that UNSPEC_LDDQU would need to be treated like UNSPEC_LOADU.
The patch made difference for 32-bit:
gcc.target/i386/sse-1.c (as you write above)
gcc.dg/torture/pr18582-1.c (UNSPEC_LDDQU)
gcc.target/i386/sse4_1-movntdqa.c (UNSPEC_MOVNTDQA)
and 64-bit also
g++.dg/torture/pr59163.C (desirable)

Now, for movntdqa, I think it accepts only aligned memory, but the MEM in there is supposed to be aligned and is created by
          if (i == memory)
            {
              /* This must be the memory operand.  */
              op = ix86_zero_extend_to_Pmode (op);
              op = gen_rtx_MEM (mode, op); 
              gcc_assert (GET_MODE (op) == mode
                          || GET_MODE (op) == VOIDmode);
            }
and there is similar code for builtins that store.
Supposedly for this we should use get_pointer_alignment (arg) and at least
set_mem_align (op, get_pointer_alignment (arg)); if it is larger than MEM_ALIGN (op).  The gcc_assert doesn't make any sense to me, result of gen_rtx_MEM (mode, op) will always have GET_MODE (op) == mode, no need to assert that and it will never have VOIDmode.
Now, if we could easily find out which of the builtins assume aligned memory (and to what extent), we should also set it, because say using _mm_stream_load_si128 with not 128-bit aligned memory is user error, so GCC should be able to assume A128 there.  I'd say the sse-1.c case is similar, isn't it?

Note that (according to my reading of the docs) e.g. movlps/movhps don't allow unaligned memory, so blindly allow any combine is wrong, but while the MEM operand in those cases is say V4SFmode, the loads or stores can have V2SFmode's alignment.  So, I don't see how ssemovu type would help us here, I think we handle all the completely unaligned loads/stores already (after the UNSPEC_LDDQU addition).  But we need some way how to determine for which instructions we should require only smaller alignment, plus tweak the two gen_rtx_MEM spots for the builtins and set mode there according to get_pointer_alignment and/or to assumed alignment of the builtins.

Perhaps add new attribute ssememalign, with default 0, which would be (maximum for all alternatives) required alignment for memory operands in the instruction pre-AVX, or 0 for GET_MODE_ALIGNMENT.  So, instructions that can handle completely unaligned loads/stores in all alternatives would have ssememalign 8,
instructions that require everything properly aligned would have default ssememalign 0, and say movlps/movhps would have ssememalign 64.  The default would be conservatively correct, so whether instructions would have ssememalign attribute would be just an optimization issue (but, if it would be non-zero, it would have to be correct).

(In reply to Jakub Jelinek from comment #17)
> Perhaps add new attribute ssememalign, with default 0, which would be
> (maximum for all alternatives) required alignment for memory operands in the
> instruction pre-AVX, or 0 for GET_MODE_ALIGNMENT.  So, instructions that can
> handle completely unaligned loads/stores in all alternatives would have
> ssememalign 8,
> instructions that require everything properly aligned would have default
> ssememalign 0, and say movlps/movhps would have ssememalign 64.  The default
> would be conservatively correct, so whether instructions would have
> ssememalign attribute would be just an optimization issue (but, if it would
> be non-zero, it would have to be correct).

This sounds like a good approach to me.

Author: jakub
Date: Wed Dec  4 11:11:24 2013
New Revision: 205661

URL: http://gcc.gnu.org/viewcvs?rev=205661&root=gcc&view=rev
Log:
	PR target/59163
	* config/i386/i386.c (ix86_legitimate_combined_insn): If for
	!TARGET_AVX there is misaligned MEM operand with vector mode
	and get_attr_ssememalign is 0, return false.
	(ix86_expand_special_args_builtin): Add get_pointer_alignment
	computed alignment and for non-temporal loads/stores also
	at least GET_MODE_ALIGNMENT as MEM_ALIGN.
	* config/i386/sse.md
	(<sse>_loadu<ssemodesuffix><avxsizesuffix><mask_name>,
	<sse>_storeu<ssemodesuffix><avxsizesuffix>,
	<sse2_avx_avx512f>_loaddqu<mode><mask_name>,
	<sse2_avx_avx512f>_storedqu<mode>, <sse3>_lddqu<avxsizesuffix>,
	sse_vmrcpv4sf2, sse_vmrsqrtv4sf2, sse2_cvtdq2pd, sse_movhlps,
	sse_movlhps, sse_storehps, sse_loadhps, sse_loadlps,
	*vec_interleave_highv2df, *vec_interleave_lowv2df,
	*vec_extractv2df_1_sse, sse2_movsd, sse4_1_<code>v8qiv8hi2,
	sse4_1_<code>v4qiv4si2, sse4_1_<code>v4hiv4si2,
	sse4_1_<code>v2qiv2di2, sse4_1_<code>v2hiv2di2,
	sse4_1_<code>v2siv2di2, sse4_2_pcmpestr, *sse4_2_pcmpestr_unaligned,
	sse4_2_pcmpestri, sse4_2_pcmpestrm, sse4_2_pcmpestr_cconly,
	sse4_2_pcmpistr, *sse4_2_pcmpistr_unaligned, sse4_2_pcmpistri,
	sse4_2_pcmpistrm, sse4_2_pcmpistr_cconly): Add ssememalign attribute.
	* config/i386/i386.md (ssememalign): New define_attr.

	* g++.dg/torture/pr59163.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/torture/pr59163.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.c
    trunk/gcc/config/i386/i386.md
    trunk/gcc/config/i386/sse.md
    trunk/gcc/testsuite/ChangeLog

Author: jakub
Date: Wed Dec  4 11:12:04 2013
New Revision: 205663

URL: http://gcc.gnu.org/viewcvs?rev=205663&root=gcc&view=rev
Log:
	PR target/59163
	* config/i386/i386.c (ix86_legitimate_combined_insn): If for
	!TARGET_AVX there is misaligned MEM operand with vector mode
	and get_attr_ssememalign is 0, return false.
	(ix86_expand_special_args_builtin): Add get_pointer_alignment
	computed alignment and for non-temporal loads/stores also
	at least GET_MODE_ALIGNMENT as MEM_ALIGN.
	* config/i386/sse.md
	(<sse>_loadu<ssemodesuffix><avxsizesuffix><mask_name>,
	<sse>_storeu<ssemodesuffix><avxsizesuffix>,
	<sse2_avx_avx512f>_loaddqu<mode><mask_name>,
	<sse2_avx_avx512f>_storedqu<mode>, <sse3>_lddqu<avxsizesuffix>,
	sse_vmrcpv4sf2, sse_vmrsqrtv4sf2, sse2_cvtdq2pd, sse_movhlps,
	sse_movlhps, sse_storehps, sse_loadhps, sse_loadlps,
	*vec_interleave_highv2df, *vec_interleave_lowv2df,
	*vec_extractv2df_1_sse, sse2_movsd, sse4_1_<code>v8qiv8hi2,
	sse4_1_<code>v4qiv4si2, sse4_1_<code>v4hiv4si2,
	sse4_1_<code>v2qiv2di2, sse4_1_<code>v2hiv2di2,
	sse4_1_<code>v2siv2di2, sse4_2_pcmpestr, *sse4_2_pcmpestr_unaligned,
	sse4_2_pcmpestri, sse4_2_pcmpestrm, sse4_2_pcmpestr_cconly,
	sse4_2_pcmpistr, *sse4_2_pcmpistr_unaligned, sse4_2_pcmpistri,
	sse4_2_pcmpistrm, sse4_2_pcmpistr_cconly): Add ssememalign attribute.
	* config/i386/i386.md (ssememalign): New define_attr.

	* g++.dg/torture/pr59163.C: New test.

Modified:
    trunk/gcc/config/i386/i386.c

Author: jakub
Date: Wed Dec  4 15:50:02 2013
New Revision: 205671

URL: http://gcc.gnu.org/viewcvs?rev=205671&root=gcc&view=rev
Log:
	PR target/59163
	* config/i386/i386.c (ix86_legitimate_combined_insn): If for
	!TARGET_AVX there is misaligned MEM operand with vector mode
	and get_attr_ssememalign is 0, return false.
	(ix86_expand_special_args_builtin): Add get_pointer_alignment
	computed alignment and for non-temporal loads/stores also
	at least GET_MODE_ALIGNMENT as MEM_ALIGN.
	* config/i386/sse.md
	(<sse>_loadu<ssemodesuffix><avxsizesuffix>,
	<sse>_storeu<ssemodesuffix><avxsizesuffix>,
	<sse2>_loaddqu<avxsizesuffix>,
	<sse2>_storedqu<avxsizesuffix>, <sse3>_lddqu<avxsizesuffix>,
	sse_vmrcpv4sf2, sse_vmrsqrtv4sf2, sse2_cvtdq2pd, sse_movhlps,
	sse_movlhps, sse_storehps, sse_loadhps, sse_loadlps,
	*vec_interleave_highv2df, *vec_interleave_lowv2df,
	*vec_extractv2df_1_sse, sse2_loadhpd, sse2_loadlpd, sse2_movsd,
	sse4_1_<code>v8qiv8hi2, sse4_1_<code>v4qiv4si2,
	sse4_1_<code>v4hiv4si2, sse4_1_<code>v2qiv2di2,
	sse4_1_<code>v2hiv2di2, sse4_1_<code>v2siv2di2, sse4_2_pcmpestr,
	*sse4_2_pcmpestr_unaligned, sse4_2_pcmpestri, sse4_2_pcmpestrm,
	sse4_2_pcmpestr_cconly, sse4_2_pcmpistr, *sse4_2_pcmpistr_unaligned,
	sse4_2_pcmpistri, sse4_2_pcmpistrm, sse4_2_pcmpistr_cconly): Add
	ssememalign attribute.
	* config/i386/i386.md (ssememalign): New define_attr.

	* g++.dg/torture/pr59163.C: New test.

Added:
    branches/gcc-4_8-branch/gcc/testsuite/g++.dg/torture/pr59163.C
Modified:
    branches/gcc-4_8-branch/gcc/ChangeLog
    branches/gcc-4_8-branch/gcc/config/i386/i386.c
    branches/gcc-4_8-branch/gcc/config/i386/i386.md
    branches/gcc-4_8-branch/gcc/config/i386/sse.md
    branches/gcc-4_8-branch/gcc/testsuite/ChangeLog

Fixed.

.