clang ignores 'aligned' attribute with 'vector_size' attribute

[tycho]

Bugzilla Link	11044
Resolution	FIXED
Resolved on	Dec 19, 2011 15:24
Version	trunk
OS	Linux
CC	@efriedma-quic

Extended Description

Below is a simple test case to show what I mean. Note the 'movaps', which is an aligned move, when the 'aligned(1)' clearly says to do unaligned moves (which GCC respects). The effect of 'movaps' in this case is a fatal fault.

Extra newlines added before each prompt for clarity.

snoonan@loki ~ $ cat test.c
#include <stdint.h>
typedef uint32_t v4 attribute ((vector_size(16), aligned(1)));
void mov_v4(v4 *x, v4 *y)
{
*x = *y;
}

snoonan@loki ~ $ clang -v
clang version 3.0 (ssh://orcus.local/scm/git/tycho/mirrors/llvm/clang.git http://llvm.org/git/clang.git a891a32d3762ee641a29c091d286f2a7432671a5)
Target: x86_64-unknown-linux-gnu
Thread model: posix

snoonan@loki ~ $ gcc -v
Using built-in specs.
COLLECT_GCC=/usr/bin/gcc
COLLECT_LTO_WRAPPER=/usr/lib/x86_64-linux-gnu/gcc/x86_64-linux-gnu/4.5.2/lto-wrapper
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --with-pkgversion='Ubuntu/Linaro 4.5.2-8ubuntu4' --with-bugurl=file:///usr/share/doc/gcc-4.5/README.Bugs --enable-languages=c,c++,fortran,objc,obj-c++ --prefix=/usr --program-suffix=-4.5 --enable-shared --enable-multiarch --with-multiarch-defaults=x86_64-linux-gnu --enable-linker-build-id --with-system-zlib --libexecdir=/usr/lib/x86_64-linux-gnu --without-included-gettext --enable-threads=posix --with-gxx-include-dir=/usr/include/c++/4.5 --libdir=/usr/lib/x86_64-linux-gnu --enable-nls --with-sysroot=/ --enable-clocale=gnu --enable-libstdcxx-debug --enable-libstdcxx-time=yes --enable-plugin --enable-gold --enable-ld=default --with-plugin-ld=ld.gold --enable-objc-gc --disable-werror --with-arch-32=i686 --with-tune=generic --enable-checking=release --build=x86_64-linux-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 4.5.2 (Ubuntu/Linaro 4.5.2-8ubuntu4)

snoonan@loki ~ $ gcc -o test-gcc.s -S test.c

snoonan@loki ~ $ clang -o test-clang.s -S test.c

snoonan@loki ~ $ head -n 50 test-*.s
==> test-clang.s <==
.file "test.c"
.text
.globl mov_v4
.align 16, 0x90
.type mov_v4,@function
mov_v4: # @​mov_v4
.Ltmp0:
.cfi_startproc

BB#0: # %entry

movq	%rdi, -8(%rsp)
movq	%rsi, -16(%rsp)
movq	-8(%rsp), %rdi
movaps	(%rsi), %xmm0
movaps	%xmm0, (%rdi)
ret

.Ltmp1:
.size mov_v4, .Ltmp1-mov_v4
.Ltmp2:
.cfi_endproc
.Leh_func_end0:

.section	".note.GNU-stack","",@progbits

==> test-gcc.s <==
.file "test.c"
.text
.globl mov_v4
.type mov_v4, @​function
mov_v4:
.LFB0:
.cfi_startproc
pushq %rbp
.cfi_def_cfa_offset 16
movq %rsp, %rbp
.cfi_offset 6, -16
.cfi_def_cfa_register 6
movq %rdi, -8(%rbp)
movq %rsi, -16(%rbp)
movq -16(%rbp), %rax
movdqu (%rax), %xmm0
movq -8(%rbp), %rax
movdqu %xmm0, (%rax)
leave
.cfi_def_cfa 7, 8
ret
.cfi_endproc
.LFE0:
.size mov_v4, .-mov_v4
.ident "GCC: (Ubuntu/Linaro 4.5.2-8ubuntu4) 4.5.2"
.section .note.GNU-stack,"",@progbits

[llvmbot]

For what it's worth, dragonegg gets this right. It seems to be a front-end
bug since clang produces this IR:

define void @​mov_v4(<4 x i32>* nocapture %x, <4 x i32>* nocapture %y) nounwind uwtable {
entry:
%tmp1 = load <4 x i32>* %y, align 16
store <4 x i32> %tmp1, <4 x i32>* %x, align 16
ret void
}

Dragonegg produces

define void @​mov_v4(<4 x i32>* nocapture %x, <4 x i32>* nocapture %y) nounwind uwtable {
entry:
%0 = load <4 x i32>* %y, align 1
store <4 x i32> %0, <4 x i32>* %x, align 1
ret void
}

[tycho]

Apparently the 'aligned' attribute isn't being handled in the processTypeAttrs() function in lib/Sema/SemaType.cpp. It doesn't even look like it permits non-POD types to have alignments, but I don't particularly know the clang code that well. So I could be wrong.

[tycho]

I just noticed there's actually a second bug here.

Even if it was using 'movups', which accounts for the alignment issue, it would still an incorrect code gen. mov(a|u)ps is titled 'Move (Aligned|Unaligned) Packed Single Precision Floating Point Values'. And this is clearly not moving floating-point values.

GCC is again correct here in using 'movdqu', which is the appropriate 128-bit unaligned integer move instruction ('Move Unaligned Double Quadword').

[efriedma-quic]

We actually are modeling this correctly in Sema; IRGen is just ignoring it. Looking into it.

(movups and movdqu aren't different in any meaningful way except that movups is shorter.)

[tycho]

Confirmed re: movaps vs movdqa. I just tested locally. It seems very odd that x86 has two instructions that are effectively identical but have different opcodes. I assumed the "floating point" in the title was significant and they perhaps did something "smart" with the representation of a floating point NaN value or something. Oh well.

LLVM is also easing the compatibility burden here, which is kind of nice: movdq[au] is SSE2, mov[au]ps is SSE. Not particularly useful for x86_64, but for x86 it might be meaningful on older hardware.

[efriedma-quic]

Partial patch follows. It fixes this case, but there are a bunch of other callers of MakeAddrLValue which don't pass in alignment, and they really all need to be audited.

Index: lib/CodeGen/CGExpr.cpp

--- lib/CodeGen/CGExpr.cpp (revision 140868)
+++ lib/CodeGen/CGExpr.cpp (working copy)
@@ -1425,7 +1425,8 @@
QualType T = E->getSubExpr()->getType()->getPointeeType();
assert(!T.isNull() && "CodeGenFunction::EmitUnaryOpLValue: Illegal type");

• LValue LV = MakeAddrLValue(EmitScalarExpr(E->getSubExpr()), T);

• unsigned Align = getContext().getTypeAlignInChars(T).getQuantity();

• LValue LV = MakeAddrLValue(EmitScalarExpr(E->getSubExpr()), T, Align);
  LV.getQuals().setAddressSpace(ExprTy.getAddressSpace());

  // We should not generate __weak write barrier on indirect reference

[lattner]

This specific test case is corrected in r146908. If there are other cases, we should handle them in their own bugs.