On 3/3/26 14:31, Robin Dapp wrote:
Hi Victor,- if (*memory_access_type == VMAT_ELEMENTWISE - || *memory_access_type == VMAT_GATHER_SCATTER_LEGACY - || *memory_access_type == VMAT_STRIDED_SLP - || *memory_access_type == VMAT_INVARIANT) + /* Even though individual VMAT_ELEMENTWISE accesses do not cause alignment + problems, loading the whole vector's worth of values in a speculative + early-break context might cross a page boundary. Set the alignment scheme + to `dr_aligned' here in order to force checking of whether such accesses + meet alignment criteria. */ + if (dr_safe_speculative_read_required (stmt_info) + && !DR_SCALAR_KNOWN_BOUNDS (STMT_VINFO_DR_INFO (stmt_info)) + && *memory_access_type != VMAT_INVARIANT) + { + if (mat_gather_scatter_p (*memory_access_type) + && !first_dr_info) + *misalignment = DR_MISALIGNMENT_UNKNOWN; + else + *misalignment = dr_misalignment (first_dr_info, vectype, *poffset); + + *alignment_support_scheme + = vect_supportable_dr_alignment + (vinfo, first_dr_info, vectype, *misalignment, + mat_gather_scatter_p (*memory_access_type)); + + if (*alignment_support_scheme != dr_unaligned_unsupported) + *alignment_support_scheme = dr_aligned; + } + else if (*memory_access_type == VMAT_ELEMENTWISE + ||*memory_access_type == VMAT_GATHER_SCATTER_LEGACY + || *memory_access_type == VMAT_STRIDED_SLP + || *memory_access_type == VMAT_INVARIANT) { *alignment_support_scheme = dr_unaligned_supported; *misalignment = DR_MISALIGNMENT_UNKNOWN;I'm afraid the new version version isn't much clearer and I'll take blame for that. What I'd go for is the hunk from v2 with the comment added and STMT_VINFO_GATHER_SCATTER removed (or did I miss something and we do need it?). The intent of my last comment was a separation of concerns: The existing if-else takes care of alignment settings for individual element misalignment and now we're adding "block misalignment" to the mix. IMHO, generally, block misalignment matters for every memory access type but we don't support most of them for early break anyway. Given that, maybe it doesn't really help code clarity to make this separation explicit. What might help slightly is leaving the if-else untouched and just adding the v2 hunk after it, as "second pass for block-level alignment". But that's very much bike-shedding at this point.
Thanks for the various bits of feedback throughout the development of this piece of work. You've not missed anything, STMT_VINFO_GATHER_SCATTER can safely be done away with. I am sorry this version of the patch isn't much clearer either. Its current form came from trying to take some of Richi's comments onboard together with your own - A balancing act I think I did poorly, so my bad for this. I did so without a clear view of the intent we wanted to convey in the way the code is organized. Having said that, if anything was left unclear until now, this particular review made your reasoning abundantly clear and, insofar as separation of concerns goes, having the "second pass for block-level alignment" logic follow the original if/else keeps the two bits of logic nice and distinct from one another. Thanks once again for your longstanding patience with this patch. I've done some refactoring in response to your feedback and am running the usual regression testing. I will have the (hopefully) final version of the patch up for review shortly. Best regards, Victor
