[ARM] Set default alignment to 64bits

[dnsampaio]

Summary

The maximum alignment used by ARM arch
is 64bits, not 128.

This could cause overaligned memory
access for 128 bit neon vector that
have unpredictable behaviour.

This fixes: https://bugs.llvm.org/show_bug.cgi?id=42668

[peter.smith]

I think that this may not apply for Android as AFAIK their ABI still requires 128-bit alignment in some cases. Adding some more reviewers from Android.

lib/Basic/Targets/ARM.cpp

311–312

I think that Android can require a higher alignment in some cases (See below). I think that this was explained in D33205

[dnsampaio]

• Set androideabi alignment to 128 bits

[dnsampaio]

Set android-abi default to 128. Added tests for android and not-android.

lib/Basic/Targets/ARM.cpp

311–312

Hi @peter.smith ,
thanks for the review, I did not know about the androidabi being 128 bits default. Updated accordingly.

[peter.smith]

Thanks for the update. Will be worth adding some reviewers from Apple to see if this change should be IsAAPCS only. I've no more further comments myself besides a small nit on style.

lib/Basic/Targets/ARM.cpp

332

Nit: suggest running clang-format over the conditional expression I think that spaces are needed around ? and :

The previous bit of code used

if (IsAAPCS && (Triple.getEnvironment() != llvm::Triple::Android))
    MaxVectorAlign = 64;

The IsAAPCS could be important here as on Macho targets IsAAPCS may not be true, see calls to setABI in ARMTargetInfo::ARMTargetInfo.

[dnsampaio]

• Joined assignments for default alignments and neon_vector alignment

[peter.smith]

test case missing A8 aside this looks ok to me. Would like to see if there are any comments from the Pacific time zone.

test/CodeGen/ARM/exception-alignment.cpp

8

Are you missing the A8 case? presumably:

store <2 x i64> <i64 1, i64 2>, <2 x i64>* [[BC]], align 8

[dnsampaio]

• Joined assignments for default alignments and neon_vector alignment
• Added missing align 8 test

[dnsampaio]

True. Thx again.

[carwil]

In D65000#1595920, @peter.smith wrote:

test case missing A8 aside this looks ok to me. Would like to see if there are any comments from the Pacific time zone.

Are you happy to approve this now @peter.smith?

[peter.smith]

I've not seen any objections so I've approved LGTM.

[pirama]

LGTM for Android.

[thakis]

cfe/trunk/test/CodeGen/ARM/exception-alignment.cpp

9 ↗

(On Diff #211941)

This fails on some bots:

http://lab.llvm.org:8011/builders/clang-atom-d525-fedora-rel/builds/26891/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Aexception-alignment.cpp

In file included from /export/users/atombot/llvm/clang-atom-d525-fedora-rel/llvm/tools/clang/test/CodeGen/ARM/exception-alignment.cpp:9:
In file included from /export/users/atombot/llvm/clang-atom-d525-fedora-rel/stage1/lib/clang/10.0.0/include/arm_neon.h:31:
In file included from /export/users/atombot/llvm/clang-atom-d525-fedora-rel/stage1/lib/clang/10.0.0/include/stdint.h:52:
In file included from /usr/include/stdint.h:26:
In file included from /usr/include/bits/libc-header-start.h:33:
In file included from /usr/include/features.h:452:
/usr/include/gnu/stubs.h:7:11: fatal error: 'gnu/stubs-32.h' file not found
# include <gnu/stubs-32.h>
          ^~~~~~~~~~~~~~~~
1 error generated.
FileCheck error: '-' is empty.
FileCheck command line:  /export/users/atombot/llvm/clang-atom-d525-fedora-rel/stage1/bin/FileCheck --check-prefixes=CHECK,A16 /export/users/atombot/llvm/clang-atom-d525-fedora-rel/llvm/tools/clang/test/CodeGen/ARM/exception-alignment.cpp

--

[thegameg]

cfe/trunk/test/CodeGen/ARM/exception-alignment.cpp

9 ↗

(On Diff #211941)

It also fails on Darwin:

http://green.lab.llvm.org/green/job/clang-stage1-cmake-RA-incremental/245/consoleFull

Command Output (stderr):
--
In file included from /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/clang/test/CodeGen/ARM/exception-alignment.cpp:9:
In file included from /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/clang-build/lib/clang/10.0.0/include/arm_neon.h:31:
In file included from /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/clang-build/lib/clang/10.0.0/include/stdint.h:52:
In file included from /usr/include/stdint.h:52:
In file included from /usr/include/sys/_types.h:32:
/usr/include/sys/cdefs.h:763:2: error: Unsupported architecture
#error Unsupported architecture
 ^
In file included from /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/llvm-project/clang/test/CodeGen/ARM/exception-alignment.cpp:9:
In file included from /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/clang-build/lib/clang/10.0.0/include/arm_neon.h:31:
In file included from /Users/buildslave/jenkins/workspace/clang-stage1-cmake-RA-incremental/clang-build/lib/clang/10.0.0/include/stdint.h:52:
In file included from /usr/include/stdint.h:52:
In file included from /usr/include/sys/_types.h:33:
/usr/include/machine/_types.h:34:2: error: architecture not supported
#error architecture not supported
 ^

[phosek]

cfe/trunk/test/CodeGen/ARM/exception-alignment.cpp

9 ↗

(On Diff #211941)

We're seeing yet another failure:

******************** TEST 'Clang :: CodeGen/ARM/exception-alignment.cpp' FAILED ********************
Script:
--
: 'RUN: at line 3';   /b/s/w/ir/k/recipe_cleanup/clang3ZiJj1/llvm_build_dir/bin/clang --target=arm-arm-none-eabi -march=armv8-a -S -emit-llvm -Os -o - /b/s/w/ir/k/llvm-project/clang/test/CodeGen/ARM/exception-alignment.cpp | /b/s/w/ir/k/recipe_cleanup/clang3ZiJj1/llvm_build_dir/bin/FileCheck --check-prefixes=CHECK,A8 /b/s/w/ir/k/llvm-project/clang/test/CodeGen/ARM/exception-alignment.cpp
: 'RUN: at line 4';   /b/s/w/ir/k/recipe_cleanup/clang3ZiJj1/llvm_build_dir/bin/clang --target=arm-linux-androideabi -march=armv8-a -S -emit-llvm -Os -o - /b/s/w/ir/k/llvm-project/clang/test/CodeGen/ARM/exception-alignment.cpp | /b/s/w/ir/k/recipe_cleanup/clang3ZiJj1/llvm_build_dir/bin/FileCheck --check-prefixes=CHECK,A16 /b/s/w/ir/k/llvm-project/clang/test/CodeGen/ARM/exception-alignment.cpp
--
Exit Code: 2

Command Output (stderr):
--
In file included from /b/s/w/ir/k/llvm-project/clang/test/CodeGen/ARM/exception-alignment.cpp:9:
In file included from /b/s/w/ir/k/recipe_cleanup/clang3ZiJj1/llvm_build_dir/lib/clang/10.0.0/include/arm_neon.h:31:
In file included from /b/s/w/ir/k/recipe_cleanup/clang3ZiJj1/llvm_build_dir/bin/../include/c++/v1/stdint.h:106:
In file included from /b/s/w/ir/k/recipe_cleanup/clang3ZiJj1/llvm_build_dir/bin/../include/c++/v1/__config:254:
/usr/include/features.h:364:12: fatal error: 'sys/cdefs.h' file not found
#  include <sys/cdefs.h>
           ^~~~~~~~~~~~~
1 error generated.
FileCheck error: '-' is empty.
FileCheck command line:  /b/s/w/ir/k/recipe_cleanup/clang3ZiJj1/llvm_build_dir/bin/FileCheck --check-prefixes=CHECK,A16 /b/s/w/ir/k/llvm-project/clang/test/CodeGen/ARM/exception-alignment.cpp

--

Can we revert this change?

[phosek]

Reverted in rL367166.

[dnsampaio]

Hi, first thanks for those that looked into this and sorry for the delay.
We have investigated the errors and seems that the test was, first in the wrong folder, inside CodeGen where it should be in CodeGenCXX and we should use clang_cc1.

[dnsampaio]

Fix test

[dnsampaio]

I have tested this in our MacOS and linux environments. @thakis @thegameg @phosek, would it be possible for you to check if this works for you?

[thegameg]

In D65000#1616368, @dnsampaio wrote:

I have tested this in our MacOS and linux environments. @thakis @thegameg @phosek, would it be possible for you to check if this works for you?

I just built trunk with this patch applied and $ ninja check-clang-codegen seems to pass. Thanks @dnsampaio!

lib/Basic/Targets/ARM.cpp

ARMTargetInfo::ARMTargetInfo(const llvm::Triple &Triple,
   }
 
   // ARM targets default to using the ARM C++ ABI.
   TheCXXABI.set(TargetCXXABI::GenericARM);
 
   // ARM has atomics up to 8 bytes
   setAtomic();
 
   // Maximum alignment for ARM NEON data types should be 64-bits (AAPCS)
+  // as well the default alignment
   if (IsAAPCS && (Triple.getEnvironment() != llvm::Triple::Android))
-    MaxVectorAlign = 64;
+    DefaultAlignForAttributeAligned = MaxVectorAlign = 64;
 
   // Do force alignment of members that follow zero length bitfields.  If
   // the alignment of the zero-length bitfield is greater than the member
   // that follows it, `bar', `bar' will be aligned as the  type of the
   // zero length bitfield.
   UseZeroLengthBitfieldAlignment = true;
 
   if (Triple.getOS() == llvm::Triple::Linux ||
       Triple.getOS() == llvm::Triple::UnknownOS)
     this->MCountName = Opts.EABIVersion == llvm::EABI::GNU
                            ? "\01__gnu_mcount_nc"
                            : "\01mcount";
 
   SoftFloatABI = llvm::is_contained(Opts.FeaturesAsWritten, "+soft-float-abi");
 }
 
 StringRef ARMTargetInfo::getABI() const { return ABI; }
 
 bool ARMTargetInfo::setABI(const std::string &Name) {
   ABI = Name;
 
   // The defaults (above) are for AAPCS, check if we need to change them.
   //
   // FIXME: We need support for -meabi... we could just mangle it into the
   // name.
   if (Name == "apcs-gnu" || Name == "aapcs16") {

test/CodeGen/ARM/exception-alignment.cpp

+// Bug: https://bugs.llvm.org/show_bug.cgi?id=42668
+// REQUIRES: arm-registered-target
+// RUN: %clang --target=arm-arm-none-eabi -march=armv8-a -S -emit-llvm -Os -o - %s | FileCheck --check-prefixes=CHECK,A8 %s
+// RUN: %clang --target=arm-linux-androideabi -march=armv8-a -S -emit-llvm -Os -o - %s | FileCheck --check-prefixes=CHECK,A16 %s
+// CHECK: [[E:%[A-z0-9]+]] = tail call i8* @__cxa_allocate_exception
+// CHECK-NEXT: [[BC:%[A-z0-9]+]] = bitcast i8* [[E]] to <2 x i64>*
+// A16-NEXT: store <2 x i64> <i64 1, i64 2>, <2 x i64>* [[BC]], align 16
+#include <arm_neon.h>
+
+int main(void) {
+  try {
+    throw vld1q_u64(((const uint64_t[2]){1, 2}));
+  } catch (uint64x2_t exc) {
+    return 0;
+  }
+  return 1;
+}
+

test/SemaCXX/warn-overaligned-type-thrown.cpp

 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.13.99 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple arm64-apple-ios10 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple arm64-apple-tvos10 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple arm64-apple-watchos4 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions -DUNDERALIGNED %s
+// RUN: %clang_cc1 -triple arm-linux-gnueabi -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions  -DUNDERALIGNED %s
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-ios12 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-tvos12 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple arm64-apple-watchos5 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
-// RUN: %clang_cc1 -triple arm-linux-gnueabi -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
+// RUN: %clang_cc1 -triple arm-linux-androideabi -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple aarch64-linux-gnueabi -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple mipsel-linux-gnu -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple mips64el-linux-gnu -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple wasm32-unknown-unknown -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple wasm64-unknown-unknown -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions %s
 // RUN: %clang_cc1 -triple x86_64-apple-macosx10.14 -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions -Wno-underaligned-exception-object -DNODIAG %s
 // RUN: %clang_cc1 -triple x86_64-windows-msvc -verify -fsyntax-only -std=c++11 -fcxx-exceptions -fexceptions -DNODIAG %s