[PATCH] D47121: [NEON] Support VLD1xN intrinsics in AArch32 mode (Clang part)

[Ivan Kosarev via Phabricator via cfe-commits]

kosarev added a comment.

Thanks for reviewing.



================
Comment at: lib/CodeGen/CGBuiltin.cpp:7865
   }
     // FIXME: Sharing loads & stores with 32-bit is complicated by the absence
     // of an Align parameter here.
----------------
SjoerdMeijer wrote:
> How about this FIXME? Is it still relevant? And does it need to be moved up? Or perhaps better: move the code back here to minimise the diff?
Yes, it's still true for the vst builtins handled below. None of the vld/vst patches removes this comment, but it should go away with whatever is the one to be committed last.

Umm, it seems leaving the vld code here wouldn't make the diff smaller?


================
Comment at: test/CodeGen/arm-neon-vld.c:4
+// RUN:     FileCheck -check-prefixes=CHECK,CHECK-A64 %s
+// RUN: %clang_cc1 -triple armv8-none-linux-gnueabi -target-feature +neon \
+// RUN:     -S -disable-O0-optnone -emit-llvm -o - %s | opt -S -mem2reg | \
----------------
SjoerdMeijer wrote:
> Should this be armv7?
There are more ARMv8 vld intrinsics that we currently support only in A64 so I was going to add tests for them here. I'm not sure if we want to test availability of NEON intrinsics for various architectures with codegen tests like this one or have some separate tests in sema.


https://reviews.llvm.org/D47121