"Yangfei (Felix)" <[email protected]> writes: > Hi! > >> -----Original Message----- >> From: Richard Sandiford [mailto:[email protected]] >> Sent: Monday, March 30, 2020 8:08 PM >> To: Yangfei (Felix) <[email protected]> >> Cc: [email protected]; [email protected] >> Subject: Re: [PATCH] ICE: in vectorizable_load, at tree-vect-stmts.c:9173 >> >> "Yangfei (Felix)" <[email protected]> writes: >> > Hi! >> >> -----Original Message----- >> >> From: Yangfei (Felix) >> >> Sent: Monday, March 30, 2020 5:28 PM >> >> To: [email protected] >> >> Cc: '[email protected]' <[email protected]> >> >> Subject: [PATCH] ICE: in vectorizable_load, at tree-vect-stmts.c:9173 >> >> >> >> Hi, >> >> >> >> New bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94398 >> >> >> >> With -mstrict-align, aarch64_builtin_support_vector_misalignment will >> >> returns false when misalignment factor is unknown at compile time. >> >> Then vect_supportable_dr_alignment returns dr_unaligned_unsupported, >> >> which triggers the ICE. I have pasted the call trace on the bug report. >> >> >> >> vect_supportable_dr_alignment is expected to return either dr_aligned >> >> or dr_unaligned_supported for masked operations. >> >> But it seems that this function only catches internal_fn >> >> IFN_MASK_LOAD & IFN_MASK_STORE. >> >> We are emitting a mask gather load here for this test case. >> >> As backends have their own vector misalignment support policy, I am >> >> supposing this should be better handled in the auto-vect shared code. >> >> >> > >> > I can only reply to comment on the bug here as my account got locked by the >> bugzilla system for now. >> >> Sorry to hear that. What was the reason? > > Looks like it got filtered by spamassassin. Admin has helped unlocked it. > >> > The way Richard (rsandifo) suggests also works for me and I agree it should >> be more consistent and better for compile time. >> > One question here: when will a IFN_MASK_LOAD/IFN_MASK_STORE be >> passed to vect_supportable_dr_alignment? >> >> I'd expect that to happen in the same cases as for unmasked load/stores. >> It certainly will for unconditional loads and stores that get masked via >> full-loop >> masking. >> >> In principle, the same rules apply to both masked and unmasked accesses. >> But for SVE, both masked and unmasked accesses should support misalignment, >> which is why I think the current target hook is probably wrong for SVE + >> -mstrict-align. > > I stopped looking into the backend further when I saw no distinction for > different type of access > in the target hook aarch64_builtin_support_vector_misalignment. > >> > @@ -8051,8 +8051,15 @@ vectorizable_store (stmt_vec_info stmt_info, >> gimple_stmt_iterator *gsi, >> > auto_vec<tree> dr_chain (group_size); >> > oprnds.create (group_size); >> > >> > - alignment_support_scheme >> > - = vect_supportable_dr_alignment (first_dr_info, false); >> > + /* Strided accesses perform only component accesses, alignment >> > + is irrelevant for them. */ >> > + if (STMT_VINFO_STRIDED_P (first_dr_info->stmt) >> > + && !STMT_VINFO_GROUPED_ACCESS (first_dr_info->stmt)) >> >> I think this should be based on memory_access_type == >> VMAT_GATHER_SCATTER instead. At this point, STMT_VINFO_* describes >> properties of the original scalar access(es) while memory_access_type >> describes the vector implementation strategy. It's the latter that matters >> here. >> >> Same thing for loads. > > Yes, I have modified accordingly. Attached please find the adapted patch. > Bootstrapped and tested on aarch64-linux-gnu. Newly add test will fail > without the patch and pass otherwise.
Looks good. OK for master. > I think I need a sponsor if this patch can go separately. Yeah, please fill in the form on: https://sourceware.org/cgi-bin/pdw/ps_form.cgi listing me as sponsor. Thanks, Richard
