[ARM] Allow codegen for Armv6m eXecute-Only (XO) sections

[stuij]

Summary

This patch moves the overall lower-bound arch restriction for Arm XO sections
from v8m to v6m. Actual implementation of code-gen for v6m will follow in
follow-up patches, which will include an implementation of relocations needed to
support this.

[simonwallis2]

llvm/lib/Target/ARM/ARMSubtarget.cpp

434 ↗

(On Diff #517898)

V6M does not have Movt.
At face value, this line looks wrong and leads to about 30 unit test fails.

[simonwallis2]

llvm/lib/Target/ARM/ARMSubtarget.cpp

434 ↗

(On Diff #517898)

I clarify: there are no test fails with this patch on its own.
The unit test fails I saw where when building this patch in conjunction with related patch https://reviews.llvm.org/D149443

[stuij]

llvm/lib/Target/ARM/ARMSubtarget.cpp

434 ↗

(On Diff #517898)

Correct, this shouldn't be here. Initially it looked like we were going to repurpose movt for v6m, but that should have been part of another patch. And now it looks like we'll handle v6m immediates separately. In any case I'll remove this.

[stuij]

addressed review comments and updated tests

[tschuett]

llvm/lib/Target/ARM/ARMSubtarget.cpp

194 ↗

(On Diff #524755)

What happens in release mode? At the top you now claim that ARMV6M is supported. Could hasV6MOps() silently return false?

[stuij]

addressed review comment

llvm/lib/Target/ARM/ARMSubtarget.cpp

194 ↗

(On Diff #524755)

This should really be caught by the frontend, but yes, I'm not sure why we use assert so often in this kind of code instead of unreachable. Changed.

[MaskRay]

clang/test/Driver/arm-execute-only.c

1–4

test/Driver tests should test just the Driver part, not sema/codegen, so we generally use -### to skip compilation.

-target has been deprecated since 3.x. Use --target=

[MaskRay]

llvm/lib/Target/ARM/ARMSubtarget.cpp

195 ↗

(On Diff #525079)

If this code path is actually reachable, a proper error reporting will be better. At the least report_fatal_error can be used. Don't use llvm_unreachable for known-reachable code paths.

[stuij]

addressed review comments

[MaskRay]

Actual implementation of code-gen for v6m will follow in follow-up patches, which will include an implementation of relocations needed to support this.

Normally we implement the feature in llvm/ first, and the clang/lib/Driver change should be the last patch.
Some users just check whether clang --target=xxx accepts a given option, not test that the feature actually works (it's sometimes non-trivial to figure out all corners as they are not compiler feature implementers.)

clang/test/Driver/arm-execute-only.c

1–4

If you don't check -cc1 options with FileCheck, you can also replace -### with -fdriver-only -Werror and drop two -NOT: patterns below. You can play with some examples to see its effect:)

If the clang -c -fdriver-only -Werror command gives no output, you can use ... 2>&1 | count 0

[stuij]

In D149444#4415306, @MaskRay wrote:

Normally we implement the feature in llvm/ first, and the clang/lib/Driver change should be the last patch.

Yes good point. I won't commit it yet. For this feature we're using Phab as a vehicle to swap patches, so we're reviewing these upstream as they become available.

clang/test/Driver/arm-execute-only.c

1–4

Very useful info. Thanks for the tips!

[stuij]

• Moved the backend parts of this patch to D149443.
• Slightly tweaked the way we call the specific arch in the tests.

[stuij]

In D149444#4425134, @stuij wrote:

• Moved the backend parts of this patch to D149443.

Wrong Phab review -^ It should be D152795.

[MaskRay]

Looks great!

clang/test/Driver/arm-execute-only.c

1–4

LGTM once the simplification is done (e.g. clang -c -fdriver-only -Werror)

[stuij]

address review comments

[stuij]

clang/test/Driver/arm-execute-only.c

1–4

yup, thanks for that. I was struggling to get rid of those NOTs.

clang/lib/Driver/ToolChains/Arch/ARM.cpp

fp16_fml_fallthrough:
   // It's not needed for multilib selection and may hide an unused
   // argument diagnostic if the code is always run.
   if (!ForAS && !ForMultilib) {
     // Supported only on ARMv6T2 and ARMv7 and above.
     // Cannot be combined with -mno-movt.
     if (Arg *A = Args.getLastArg(options::OPT_mexecute_only, options::OPT_mno_execute_only)) {
       if (A->getOption().matches(options::OPT_mexecute_only)) {
         if (getARMSubArchVersionNumber(Triple) < 7 &&
-            llvm::ARM::parseArch(Triple.getArchName()) != llvm::ARM::ArchKind::ARMV6T2)
+            llvm::ARM::parseArch(Triple.getArchName()) != llvm::ARM::ArchKind::ARMV6T2 &&
+            llvm::ARM::parseArch(Triple.getArchName()) != llvm::ARM::ArchKind::ARMV6M)
               D.Diag(diag::err_target_unsupported_execute_only) << Triple.getArchName();
         else if (Arg *B = Args.getLastArg(options::OPT_mno_movt))
           D.Diag(diag::err_opt_not_valid_with_opt)
               << A->getAsString(Args) << B->getAsString(Args);
         Features.push_back("+execute-only");
       }
     }
   }

clang/test/Driver/arm-execute-only.c

-// RUN: not %clang -c -target thumbv6m-eabi -mexecute-only %s 2>&1 | \
+// RUN: %clang -c -fdriver-only -Werror --target=arm-arm-none-eabi \
+// RUN:   -march=armv6-m -mexecute-only %s 2>&1 | count 0
+
+// RUN: %clang -### -c --target=arm-arm-none-eabi -march=armv6 -mexecute-only %s 2>&1 |    \
 // RUN:   FileCheck --check-prefix CHECK-EXECUTE-ONLY-NOT-SUPPORTED %s
-// CHECK-EXECUTE-ONLY-NOT-SUPPORTED: error: execute only is not supported for the thumbv6m sub-architecture
+// CHECK-EXECUTE-ONLY-NOT-SUPPORTED: error: execute only is not supported for the armv6 sub-architecture
 
-// RUN: not %clang -target armv8m.main-eabi -mexecute-only -mno-movt %s 2>&1 \
+// RUN: %clang -### --target=arm-arm-none-eabi -march=armv8-m.main -mexecute-only -mno-movt %s 2>&1 \
 // RUN:    | FileCheck %s -check-prefix CHECK-EXECUTE-ONLY-NO-MOVT
 // CHECK-EXECUTE-ONLY-NO-MOVT: error: option '-mexecute-only' cannot be specified with '-mno-movt'
 
-// RUN: %clang -target armv7m-eabi -x assembler -mexecute-only %s -c -### 2>&1 \
+// RUN: %clang -### --target=arm-arm-none-eabi -march=armv7-m -x assembler -mexecute-only %s -c -### 2>&1 \
 // RUN:    | FileCheck %s --check-prefix=CHECK-NO-EXECUTE-ONLY-ASM
 // CHECK-NO-EXECUTE-ONLY-ASM: warning: argument unused during compilation: '-mexecute-only'
 
 // -mpure-code flag for GCC compatibility
-// RUN: not %clang -c -target thumbv6m-eabi -mpure-code %s 2>&1 | \
+// RUN: %clang -### -c --target=arm-arm-none-eabi -march=armv6 -mpure-code %s 2>&1 | \
 // RUN:   FileCheck --check-prefix CHECK-EXECUTE-ONLY-NOT-SUPPORTED %s
 
-// RUN: not %clang -target armv8m.main-eabi -mpure-code -mno-movt %s 2>&1 \
+// RUN: %clang -### --target=arm-arm-none-eabi -march=armv8-m.main -mpure-code -mno-movt %s 2>&1 \
 // RUN:    | FileCheck %s -check-prefix CHECK-PURE-CODE-NO-MOVT
 // CHECK-PURE-CODE-NO-MOVT: error: option '-mpure-code' cannot be specified with '-mno-movt'