Paths
| Differential D88396
[X86] Support replacing aligned vector moves with unaligned moves when avx is enabled. (off by default) AbandonedPublic Authored by LuoYuanke on Sep 28 2020, 1:35 AM.
Details Summary With AVX the performance for aligned vector move and unaligned vector move on X86 are the same if the address is aligned. However if the address is not aligned, aligned vector move raise exception while unaligned vector move can still run. To be conservative, llvm option "x86-enable-unaligned-vector-move" is added to enable this preference. Change-Id: I85ab9749013d7e1abb237e03bc22eeacfd37836a
Diff Detail
Event TimelineComment Actions What issue is this fixing?
That sounds like either a miscompile happened along the way, or the original source code had UB to begin with. Comment Actions
It can avoid segment fault when unaligned pointer is casted. #include <immintrin.h>
extern __m128 value;
void add(void* pointer) {
value = _mm_add_ps(value,*(__m128*)pointer);
}Comment Actions
That is undefined behaviour: This revision now requires changes to proceed.Sep 28 2020, 2:00 AM Comment Actions As @lebedev.ri has said, I don't think this is a good idea and if its happening it sounds like you have an underlying bug in your code that the sanitizers would probably help you with. Comment Actions Ignoring the motivating code here for a second. Using aligned load/store instructions with AVX is a little weird. If we fold the load into an arithmetic op, with AVX it doesn't have to be aligned so using the folded instruction suppresses the fault check. This is different than SSE where we can only fold aligned loads except on AMD CPUs. So AVX provides an inconsistent faulting experience. But if we were going to change that I'd hunt down all the places we check the alignment and remove them and take the code size reduction in the compiler instead of adding a new pass. Comment Actions If the address is random, during validation the address is aligned and sanitizers tool doesn't notice it. When run in real time, it crashes randomly. There is no harm to replace movaps with movups, and it can avoid some crash issue. Is it doable to add an option to let user choose movups or movaps? Comment Actions
If you change void* to char*, you get clang diagnostic: <source>:4:38: warning: cast from 'char *' to '__m128 *' increases required alignment from 1 to 16 [-Wcast-align]
__m128 value = _mm_add_ps(value,*(__m128*)pointer);
^~~~~~~~~~~~~~~~So this should in principle not require sanitizers. Comment Actions I didn't get the error at https://godbolt.org/z/8aGhd5. Another example may like this, an float array is packed in a struct. #include <immintrin.h>
__m128 value;
typedef struct _data_str {
int header;
float src[400];
} data_t;
data_t data;
void add(__m128* pointer) {
value = _mm_add_ps(value, *pointer);
}
void foo() {
for (int i = 0; i < 400; i += 4)
add((__m128*)(&data.src[i]));
}Comment Actions
Compiling for SSE this code will likely use the memory form of addps which will fault on the misalignment. I know this patch only targets AVX. I don’t think you can motivate this change by showing what code you want to accept if the code would crash when compiled with the default SSE2 target. Comment Actions
Yes, this patch only targets AVX.
Comment Actions
Note that even if x86 codegen will always emit unaligned ops (which will cause new questions/bugreports), Comment Actions
Sorry, what does 'UB' means? Why cause 'miscompile', compiler still think the address is aligned. Selecting movups doesn't break compiler assumption. Is there any reason movaps is better than movups? To detect the alignment exception? Comment Actions
undefined behavior
That is very precisely my point.
Comment Actions
Why we need to detect the alignment exception? This is just like assert, it can be done in debug mode. So can we select movaps in debug build, and select movups in non-debug build? Comment Actions
I never said that. I'm only saying that the original source code has undefined behavior, Comment Actions Add llvm option "-enable-x86-movaps-to-movups" to enable movups preference.
Comment Actions Alignment information isn’t just used to select aligned or unaligned instructions. It’s also used by alias analysis for example. If the compiler is being told incorrect alignment it could cause incorrect optimization in other parts of the compiler. Comment Actions
(That's what i've been saying all this time...) LuoYuanke retitled this revision from [X86] Replace movaps with movups when avx is enabled. to [X86] Replace aligned vector move with unaligned move when avx is enabled.. LuoYuanke retitled this revision from [X86] Replace aligned vector move with unaligned move when avx is enabled. to [X86] Replace aligned vector move with unaligned move when avx is enabled.. Comment ActionsAddress Pengfei's comments.
lebedev.ri retitled this revision from [X86] Replace aligned vector move with unaligned move when avx is enabled. to [X86] Support replacing aligned vector moves with unaligned moves when avx is enabled. (off by default).Oct 9 2020, 11:16 PM Comment Actions LGTM. Thanks! Comment Actions
I still retain my original opinion that this is trying to paper over broken source code, As i see it, there are 5 options:
I strongly suggest that an option 4 be taken. Comment Actions
I think it is friendly for compiler to provide an opportunity to let user decide whether he/she prefer aligned load or unaligned load. As I know some processor also have some control register to control raising or suppressing exception on unaligned memory access. Leaving the decision to user doesn't harm any existing behavior. For X86 we have a choice to select aligned instruction or unaligned instruction. What if some processor only have instruction that don't raise exception on unaligned memory access? Comment Actions
I think we are talking past each other.
Revision Contents
Diff 297401 llvm/lib/Target/X86/CMakeLists.txt
llvm/lib/Target/X86/X86.h
llvm/lib/Target/X86/X86TargetMachine.cpp
llvm/lib/Target/X86/X86UnalignedVectorMoves.cpp
llvm/test/CodeGen/X86/O0-pipeline.ll
llvm/test/CodeGen/X86/avx-unaligned-load-store.ll
llvm/test/CodeGen/X86/avx512-unaligned-load-store.ll
llvm/test/CodeGen/X86/avx512vl-unaligned-load-store.ll
llvm/test/CodeGen/X86/opt-pipeline.ll
| ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
sorting