Skip to content

[Driver,AArch64] Remove AArch32-specific -m[no-]unaligned-access #85441

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Driver,AArch64] Remove AArch32-specific -m[no-]unaligned-access #85441

wants to merge 1 commit into from
+6 −29

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Mar 15, 2024

Follow-up to #85350: GCC only supports -m[no-]strict-align for AArch64
and rejects adding -m[no-]unaligned-access aliases. We inapropriated
supported -m[no-]unaligned-access as aliases for non-AArch64 due to an
early design issue (a146a48).

Since the behavior has been longtime for ARM, keep ARM unchanged but
tighten up the behavior for AArch64.

@MaskRay
[𝘀𝗽𝗿] initial version
bf21dc2 
Created using spr 1.3.5-bogner
@MaskRay MaskRay requested a review from smithp35 March 15, 2024 18:06
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:AArch64 clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Mar 15, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 15, 2024 •
edited
Loading

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Fangrui Song (MaskRay)

Changes

Follow-up to #85350: GCC only supports -m[no-]strict-align for AArch64
and rejects adding -m[no-]unaligned-access aliases. We inapropriated
supported -m[no-]unaligned-access as aliases for non-AArch64 due to an
early design issue (a146a48).

Since the behavior has been longtime for ARM, keep ARM unchanged but
tighten up the behavior for AArch64.

Full diff: https://github.com/llvm/llvm-project/pull/85441.diff

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+1-1)
  • (modified) clang/lib/Driver/ToolChains/Arch/AArch64.cpp (+2-7)
  • (modified) clang/test/Driver/arm-alignment.c (+3-21)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e1743368b157e0..a9e3be617d0e74 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -191,7 +191,7 @@ Removed Compiler Flags
 -------------------------
 
 - The ``-freroll-loops`` flag has been removed. It had no effect since Clang 13.
-- ``-m[no-]unaligned-access`` is removed for RISC-V and LoongArch.
+- ``-m[no-]unaligned-access`` is removed for AArch64, RISC-V, and LoongArch.
   ``-m[no-]strict-align``, also supported by GCC, should be used instead.
   (`#85350 <https://github.com/llvm/llvm-project/pull/85350>`_.)
 
diff --git a/clang/lib/Driver/ToolChains/Arch/AArch64.cpp b/clang/lib/Driver/ToolChains/Arch/AArch64.cpp
index 3e6e29584df3ac..cac5ab5f369e57 100644
--- a/clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -321,13 +321,8 @@ void aarch64::getAArch64TargetFeatures(const Driver &D,
     }
   }
 
-  if (Arg *A = Args.getLastArg(
-          options::OPT_mstrict_align, options::OPT_mno_strict_align,
-          options::OPT_mno_unaligned_access, options::OPT_munaligned_access)) {
-    if (A->getOption().matches(options::OPT_mstrict_align) ||
-        A->getOption().matches(options::OPT_mno_unaligned_access))
-      Features.push_back("+strict-align");
-  } else if (Triple.isOSOpenBSD())
+  if (Args.hasFlag(options::OPT_mstrict_align, options::OPT_mno_strict_align,
+                   Triple.isOSOpenBSD()))
     Features.push_back("+strict-align");
 
   if (Args.hasArg(options::OPT_ffixed_x1))
diff --git a/clang/test/Driver/arm-alignment.c b/clang/test/Driver/arm-alignment.c
index 8c915477af9aff..f7db966f9818ab 100644
--- a/clang/test/Driver/arm-alignment.c
+++ b/clang/test/Driver/arm-alignment.c
@@ -37,19 +37,13 @@
 // RUN: %clang -target thumbv8m.base-none-gnueabi -### %s 2> %t
 // RUN: FileCheck --check-prefix CHECK-ALIGNED-ARM <%t %s
 
-// RUN: %clang --target=aarch64 -munaligned-access -### %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-UNALIGNED-AARCH64 < %t %s
-
-// RUN: %clang --target=aarch64 -mstrict-align -munaligned-access -### %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-UNALIGNED-AARCH64 < %t %s
-
-// RUN: %clang --target=aarch64 -mno-unaligned-access -munaligned-access -### %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-UNALIGNED-AARCH64 < %t %s
+// RUN: not %clang --target=aarch64 -munaligned-access -mno-unaligned-access -### %s 2>&1 | FileCheck %s --check-prefix=ERR
+// ERR: error: unsupported option '-munaligned-access' for target 'aarch64'
+// ERR: error: unsupported option '-mno-unaligned-access' for target 'aarch64'
 
 // CHECK-UNALIGNED-ARM-NOT: "-target-feature" "+strict-align"
 // CHECK-UNALIGNED-AARCH64-NOT: "-target-feature" "+strict-align"
 
-
 // RUN: %clang -target arm-none-gnueabi -mno-unaligned-access -### %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-ALIGNED-ARM < %t %s
 
@@ -83,21 +77,9 @@
 // RUN: %clang -target armv6m-netbsd-eabi -### %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-ALIGNED-ARM < %t %s
 
-// RUN: %clang --target=aarch64 -mno-unaligned-access -### %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s
-
 // RUN: %clang --target=aarch64 -mstrict-align -### %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s
 
-// RUN: %clang --target=aarch64 -munaligned-access -mno-unaligned-access -### %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s
-
-// RUN: %clang --target=aarch64 -munaligned-access -mstrict-align -### %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s
-
-// RUN: %clang --target=aarch64 -mkernel -mno-unaligned-access -### %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s
-
 // RUN: %clang -target aarch64-unknown-openbsd -### %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s

@llvmbot
Copy link
Member

llvmbot commented Mar 15, 2024

@llvm/pr-subscribers-backend-aarch64

Author: Fangrui Song (MaskRay)

Changes

Follow-up to #85350: GCC only supports -m[no-]strict-align for AArch64
and rejects adding -m[no-]unaligned-access aliases. We inapropriated
supported -m[no-]unaligned-access as aliases for non-AArch64 due to an
early design issue (a146a48).

Since the behavior has been longtime for ARM, keep ARM unchanged but
tighten up the behavior for AArch64.

Full diff: https://github.com/llvm/llvm-project/pull/85441.diff

3 Files Affected:

  • (modified) clang/docs/ReleaseNotes.rst (+1-1)
  • (modified) clang/lib/Driver/ToolChains/Arch/AArch64.cpp (+2-7)
  • (modified) clang/test/Driver/arm-alignment.c (+3-21)
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index e1743368b157e0..a9e3be617d0e74 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -191,7 +191,7 @@ Removed Compiler Flags
 -------------------------
 
 - The ``-freroll-loops`` flag has been removed. It had no effect since Clang 13.
-- ``-m[no-]unaligned-access`` is removed for RISC-V and LoongArch.
+- ``-m[no-]unaligned-access`` is removed for AArch64, RISC-V, and LoongArch.
   ``-m[no-]strict-align``, also supported by GCC, should be used instead.
   (`#85350 <https://github.com/llvm/llvm-project/pull/85350>`_.)
 
diff --git a/clang/lib/Driver/ToolChains/Arch/AArch64.cpp b/clang/lib/Driver/ToolChains/Arch/AArch64.cpp
index 3e6e29584df3ac..cac5ab5f369e57 100644
--- a/clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -321,13 +321,8 @@ void aarch64::getAArch64TargetFeatures(const Driver &D,
     }
   }
 
-  if (Arg *A = Args.getLastArg(
-          options::OPT_mstrict_align, options::OPT_mno_strict_align,
-          options::OPT_mno_unaligned_access, options::OPT_munaligned_access)) {
-    if (A->getOption().matches(options::OPT_mstrict_align) ||
-        A->getOption().matches(options::OPT_mno_unaligned_access))
-      Features.push_back("+strict-align");
-  } else if (Triple.isOSOpenBSD())
+  if (Args.hasFlag(options::OPT_mstrict_align, options::OPT_mno_strict_align,
+                   Triple.isOSOpenBSD()))
     Features.push_back("+strict-align");
 
   if (Args.hasArg(options::OPT_ffixed_x1))
diff --git a/clang/test/Driver/arm-alignment.c b/clang/test/Driver/arm-alignment.c
index 8c915477af9aff..f7db966f9818ab 100644
--- a/clang/test/Driver/arm-alignment.c
+++ b/clang/test/Driver/arm-alignment.c
@@ -37,19 +37,13 @@
 // RUN: %clang -target thumbv8m.base-none-gnueabi -### %s 2> %t
 // RUN: FileCheck --check-prefix CHECK-ALIGNED-ARM <%t %s
 
-// RUN: %clang --target=aarch64 -munaligned-access -### %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-UNALIGNED-AARCH64 < %t %s
-
-// RUN: %clang --target=aarch64 -mstrict-align -munaligned-access -### %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-UNALIGNED-AARCH64 < %t %s
-
-// RUN: %clang --target=aarch64 -mno-unaligned-access -munaligned-access -### %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-UNALIGNED-AARCH64 < %t %s
+// RUN: not %clang --target=aarch64 -munaligned-access -mno-unaligned-access -### %s 2>&1 | FileCheck %s --check-prefix=ERR
+// ERR: error: unsupported option '-munaligned-access' for target 'aarch64'
+// ERR: error: unsupported option '-mno-unaligned-access' for target 'aarch64'
 
 // CHECK-UNALIGNED-ARM-NOT: "-target-feature" "+strict-align"
 // CHECK-UNALIGNED-AARCH64-NOT: "-target-feature" "+strict-align"
 
-
 // RUN: %clang -target arm-none-gnueabi -mno-unaligned-access -### %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-ALIGNED-ARM < %t %s
 
@@ -83,21 +77,9 @@
 // RUN: %clang -target armv6m-netbsd-eabi -### %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-ALIGNED-ARM < %t %s
 
-// RUN: %clang --target=aarch64 -mno-unaligned-access -### %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s
-
 // RUN: %clang --target=aarch64 -mstrict-align -### %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s
 
-// RUN: %clang --target=aarch64 -munaligned-access -mno-unaligned-access -### %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s
-
-// RUN: %clang --target=aarch64 -munaligned-access -mstrict-align -### %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s
-
-// RUN: %clang --target=aarch64 -mkernel -mno-unaligned-access -### %s 2> %t
-// RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s
-
 // RUN: %clang -target aarch64-unknown-openbsd -### %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-ALIGNED-AARCH64 < %t %s

@MaskRay MaskRay requested a review from jroelofs March 15, 2024 18:08
@MaskRay
Copy link
Member Author

MaskRay commented Mar 15, 2024 •
edited
Loading

If Apple platforms need this option, I can keep it recognized but only for Apple.

For ELF platforms, we should remove the GCC-unsupported-and-rejected aliases, so that software written for one compiler will not unnecessarily have portability problems for the other.

smithp35
smithp35 reviewed Mar 18, 2024
View reviewed changes
Copy link
Collaborator

@smithp35 smithp35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible I would prefer to keep -m[no-]unaligned-access for AArch64.

The history of this option name derives from Arm's proprietary compiler https://developer.arm.com/documentation/dui0472/m/Compiler-Command-line-Options/--unaligned-access----no-unaligned-access which has been carried forward for the LLVM based Arm Compiler https://developer.arm.com/documentation/101754/0621/armclang-Reference/armclang-Command-line-Options/-munaligned-access---mno-unaligned-access?lang=en

Yes the proprietary compiler can always put this back as a downstream change. However we are trying to introduce more use of upstream clang and it would help migration of these projects if they didn't need to change.

@jroelofs
Copy link
Contributor

If Apple platforms need this option, I can keep it recognized but only for Apple.

I don't see any projects that use -m(no-)unaligned-access, but it could be that I don't have permissions on ones that do. I'll give our qualification team a heads-up if/when you land it, and if we need to we can have an exception for *-apple-* triples or something. We'll let you know.

@MaskRay
Copy link
Member Author

MaskRay commented Mar 18, 2024 •
edited
Loading

If possible I would prefer to keep -m[no-]unaligned-access for AArch64.

The history of this option name derives from Arm's proprietary compiler developer.arm.com/documentation/dui0472/m/Compiler-Command-line-Options/--unaligned-access----no-unaligned-access which has been carried forward for the LLVM based Arm Compiler developer.arm.com/documentation/101754/0621/armclang-Reference/armclang-Command-line-Options/-munaligned-access---mno-unaligned-access?lang=en

Yes the proprietary compiler can always put this back as a downstream change. However we are trying to introduce more use of upstream clang and it would help migration of these projects if they didn't need to change.

Thanks for the comments. The first link gives --unaligned_access, --no_unaligned_access, which Clang doesn't support.
Does the second link mean AArch32 or AArch64? I thinks there may be strong motivation to keep both -m[no-]strict-align for AArch32 (-mno-strict-align was a recent introduction by LoongArch folks) but very little for AArch64 (since GCC has always been rejecting -m[no-]unaligned-access).

Part of the motivation behind the change and #85350 is to discourage future ports (including existing RISC-V/LoongArch) to create aliases for architectures that don't need the aliases.

@smithp35
Copy link
Collaborator

If possible I would prefer to keep -m[no-]unaligned-access for AArch64.
The history of this option name derives from Arm's proprietary compiler developer.arm.com/documentation/dui0472/m/Compiler-Command-line-Options/--unaligned-access----no-unaligned-access which has been carried forward for the LLVM based Arm Compiler developer.arm.com/documentation/101754/0621/armclang-Reference/armclang-Command-line-Options/-munaligned-access---mno-unaligned-access?lang=en
Yes the proprietary compiler can always put this back as a downstream change. However we are trying to introduce more use of upstream clang and it would help migration of these projects if they didn't need to change.

Thanks for the comments. The first link gives --unaligned_access, --no_unaligned_access, which Clang doesn't support.

Yes the first link is just showing the history of the name. The --unaligned_access became -munaligned-access in clang/gcc for Arm.

Does the second link mean AArch32 or AArch64?

It means both.

I thinks there may be strong motivation to keep both -m[no-]strict-align for AArch32 (-mno-strict-align was a recent introduction by LoongArch folks) but very little for AArch64 (since GCC has always been rejecting -m[no-]unaligned-access).

Part of the motivation behind the change and #85350 is to discourage future ports (including existing RISC-V/LoongArch) to create aliases for architectures that don't need the aliases.

I would just like to keep the -mno-unaligned-access alias for AArch64 if at all possible, I've no problem with it not aliasing for all targets. I think existing code coming from GCC or needing to be compatible with GCC will use -mstrict-align and -mno-strict-align and will be unaffected, however looking at the history this option has been supported in clang for AArch64 for almost 10 years. In our proprietary toolchain we've consistently used -mno-unaligned-access for AArch64. As mentioned previously we can always downstream patch that, although it adds yet another item to the migration guide to move people to upstream clang.

Tracing the history back it looks like AArch64 -mno-unaligned-access support was added in 2014 with

commit e5cee260ce07f96a8bc8f82905bc319cc33106fe
Author: Kevin Qin <[email protected]>
Date:   Tue May 6 09:51:32 2014 +0000

    [PATCH] [ARM64] Enable alignment control option in front-end for ARM64.

    This patch is to get "-mno-unaligned-access" and "-munaligned-access"
    work in front-end for ARM64 target.

    llvm-svn: 208075

Checking our own issue tracker I've found our own internal ticket. Which doesn't add much information, it does have:

ARM64 had one back-end option "arm64-strict-align" to enable strict align mode, but didn't support any command line option in front-end. This patch allows users use "-mno-unaligned-access" and "-munaligned-access" in front-end to directly control alignment settings.

@MaskRay
Copy link
Member Author

MaskRay commented Mar 18, 2024

Thanks for the comment. I'll abandon this.

@MaskRay MaskRay closed this Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smithp35 smithp35 smithp35 left review comments

@jroelofs jroelofs Awaiting requested review from jroelofs

Assignees
No one assigned
Labels
backend:AArch64 clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@MaskRay @llvmbot @jroelofs @smithp35