This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Set default alignment to 64bits
ClosedPublic

Authored by dnsampaio on Jul 19 2019, 9:01 AM.

Diff Detail

Event Timeline

dnsampaio created this revision.Jul 19 2019, 9:01 AM
peter.smith added a subscriber: 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 updated this revision to Diff 211034.Jul 22 2019, 2:45 AM
  • Set androideabi alignment to 128 bits
dnsampaio marked 2 inline comments as done.Jul 22 2019, 3:07 AM

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.

dnsampaio marked an inline comment as done.Jul 22 2019, 3:14 AM

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 updated this revision to Diff 211103.Jul 22 2019, 8:06 AM
  • Joined assignments for default alignments and neon_vector alignment

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
9

Are you missing the A8 case? presumably:

store <2 x i64> <i64 1, i64 2>, <2 x i64>* [[BC]], align 8
dnsampaio updated this revision to Diff 211132.Jul 22 2019, 9:36 AM
  • Joined assignments for default alignments and neon_vector alignment
  • Added missing align 8 test
dnsampaio marked an inline comment as done.Jul 22 2019, 9:36 AM

True. Thx again.

carwil added a comment.EditedJul 25 2019, 5:42 AM

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 accepted this revision.Jul 25 2019, 8:22 AM

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

This revision is now accepted and ready to land.Jul 25 2019, 8:22 AM
pirama accepted this revision.Jul 25 2019, 10:20 AM

LGTM for Android.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 26 2019, 8:04 AM
thakis added a subscriber: thakis.Jul 26 2019, 11:16 AM
thakis added inline comments.
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 added inline comments.
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 added a subscriber: phosek.Jul 26 2019, 4:51 PM
phosek added inline comments.
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?

dnsampaio reopened this revision.Aug 6 2019, 2:01 AM

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.

This revision is now accepted and ready to land.Aug 6 2019, 2:01 AM

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 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!

This revision was automatically updated to reflect the committed changes.