Closed stephenhines closed 4 years ago
I think this is a duplicate of llvm/llvm-project#45169 , which has been fixed. I couldn't reproduce this anymore, so resolving this.
At the the least, do we need to add 3 new cases for these 3 new intrinsics to CombineUpdateBase? Maybe add those cases to the
continue
to skip these for now?
Looks like doing so doesn't fix the crash.
D47121 adds 3 new intrinsics:
CombineBaseUpdate has logic: if (isIntrinsic) {
When git bisecting this, this points to the commit that enabled Neon by default on Android. When git bisecting this but instead for a armv7a-linux-gnueabihf target, this points to the following commit: 9c40c0ad0ca49a0b4eff20bf85b088daecb0014f is the first bad commit commit 9c40c0ad0ca49a0b4eff20bf85b088daecb0014f Author: Ivan A. Kosarev ikosarev@accesssoftek.com Date: Sat Jun 2 17:42:59 2018 +0000
[NEON] Support VLD1xN intrinsics in AArch32 mode (Clang part)
We currently support them only in AArch64. The NEON Reference,
however, says they are 'ARMv7, ARMv8' intrinsics.
Differential Revision: https://reviews.llvm.org/D47121
llvm-svn: 333829
clang/include/clang/Basic/arm_neon.td | 16 +- clang/lib/CodeGen/CGBuiltin.cpp | 57 +- clang/test/CodeGen/aarch64-neon-intrinsics.c | 1276 +---------------------- clang/test/CodeGen/arm-neon-vld.c | 1411 ++++++++++++++++++++++++++ 4 files changed, 1457 insertions(+), 1303 deletions(-) create mode 100644 clang/test/CodeGen/arm-neon-vld.c
So, this points to the commit that introduced support for these intrinsics for AArch32 (ARM) that was earlier enabled for AArch64 (see http://llvm.org/viewvc/llvm-project?view=revision&revision=194990)
When running the reproducer under a debugger, it points to an assertion in function CombineBaseUpdate in ARMISelLowering.cpp, indicating that a switch isn't handling the newly introduced intrinsics.
It looks very much like a number of pieces of functionality were not introduced as part of https://reviews.llvm.org/D47121 that should have been introduced. When comparing to the commit that introduced support for these intrinsics in AArch64 there are e.g. no additions to function CombineBaseUpdate in ARMISelLowering.cpp in https://reviews.llvm.org/D47121, whereas there are additions for them in the same function in AArch64ISelLowering.cpp, see http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp?view=diff&r1=194989&r2=194990&pathrev=194990).
To fix this, it seems that a comparison should be made for where in the AArch64 backend additions were made for the new intrinsics under commit http://llvm.org/viewvc/llvm-project?view=revision&revision=19499; for which equivalent additions in the ARM backend are missing under patch https://reviews.llvm.org/D47121.
Extended Description
I reduced down the following crash from an NDK bug report (https://github.com/android/ndk/issues/1303). https://godbolt.org/z/MXuui4 shows the crash as well. This isn't specifically a regression, but is a crash on what appears to be valid code.