[ARM] Remove FeatureNoARM implies ModeThumb.

[fhahn]

Summary

By removing FeatureNoARM implies ModeThumb, we can detect cases where a
function's target-features contain -thumb-mode (enables ARM codegen for the
function), but the architecture does not support ARM mode. Previously, the
implication caused the FeatureNoARM bit to be cleared for functions with
-thumb-mode, making the assertion in ARMSubtarget::ARMSubtarget [1]
pointless for such functions.

This assertion is the only guard against generating ARM code for
architectures without ARM codegen support. Is there a place where we
could easily generate error messages for the user? At the moment, we
would generate ARM code for Thumb-only architectures. X86 has the same
behavior as ARM, as in it only has an assertion and no error message,
but I think for ARM an error message would be helpful. What do you
think?

For the example below, llc -mtriple=armv7m-eabi test.ll -o - will
generate ARM assembler (or fail with an assertion error with this patch).
Note that if we run the resulting assembler through llvm-mc, we get
an appropriate error message, but not when codegen is handled
through clang.

define void @bar() #0 {
entry:
  ret void
}

attributes #0 = { "target-features"="-thumb-mode" }

[1] https://github.com/llvm-mirror/llvm/blob/c1f7b54cef62e9c8aa745d40bea146a167bf844e/lib/Target/ARM/ARMSubtarget.cpp#L147

[fhahn]

The assertion on X86 and ARM (with this patch) can be triggered using clang and the target function attribute, e.g.

clang -target i386-unknown-linux -march=pentium2 test.c -c

__attribute__((target("64bit-mode"))) int bar(int a, int b)
{
    return a + b % a;
}
__attribute__((target("32bit-mode"))) int foo(int a, int b)
{
    return a + b % a;
}

and

clang -target armv7m-eabi -c test-arm.c

__attribute__((target("thumb"))) int bar(int a, int b)
{
    return a + b % a;
}
__attribute__((target("arm"))) int foo(int a, int b)
{
    return a + b % a;
}

Mixed ARM/Thumb LTO could also expose this problem.

[efriedma]

In terms of detecting the error, ARMBaseTargetMachine::getSubtargetImpl is probably the right place: that's where we gather the target-cpu and target-features to actually compute the target for a given function.

[fhahn]

There used to be an error message in ARMBaseTargetMachine if the selected ISA is ARM, but the CPU only supports Thumb. The check was removed in https://github.com/llvm-mirror/llvm/commit/26e9879a19bb6300bcbe5242681388d2a05e76cd by @echristo with a suggestion to do error reporting in the frontend. At the moment it does not look like Clang has access to the architecture/CPU feature information available in LLVM/lib/Target/ARM, which would be required to report the error in Clang. When we want to remove the error on a per-function basis, it gets even harder to do in the cases I mentioned above. To get proper error messages during LTO, we would have to add similar logic to the IR linker I think.

IMO it would make sense to have an error message in ARMBaseTargetMachine::getSubtargetImpl (as Eli suggested) rather than handing off the responsibilities to the frontends,. What do you think?

[fhahn]

I've created D35569, in case we want to go with an error message, which I think makes sense, as we emit a similar error for hand-written assembler.

Either way I think we have to remove the implication, as for functions with "-thumb-mode" in target-features, we also set "-no-arm", as target-features are added after the CPU/architecture features haven been added. For such functions, we would then generate ARM code even though the target does not support ARM execution mode, which is a violation of the ACLE I think, which states

The implementation must generate code in the required state unless it is impossible to do so (section 7.3 Target selection).

The alternative to generating an error would be to switch -thumb-mode to +thumb-mode in that case. I think recent versions of GCC report an error in that case though, which seems sensible to me.

[efriedma]

We want to make sure "-mcpu=cortex-m0" implies -mthumb in clang... but yes, we probably don't want to mess with the target features in the backend.

In terms of cleaning this up, we should probably have "FeatureARM", for CPUs which support ARM-mode execution, rather than "FeatureNoARM"; negative features are confusing.

[fhahn]

In D35569#814990, @efriedma wrote:

We want to make sure "-mcpu=cortex-m0" implies -mthumb in clang... but yes, we probably don't want to mess with the target features in the backend.

I agree that this would be ideal from the user's perspective. I am not sure what the best way to implement that would be though. The information which CPUs support which modes is available in the backend, but I am not sure how we would make use of that information in Clang. And duplicating the information in Clang does not seem ideal. Any suggestions how we could tackle that?

In terms of cleaning this up, we should probably have "FeatureARM", for CPUs which support ARM-mode execution, rather than "FeatureNoARM"; negative features are confusing.

That sounds like a good idea. I'll update this patch tomorrow, unless there are any objections.

[fhahn]

In D35569#815009, @fhahn wrote:

In D35569#814990, @efriedma wrote:

In terms of cleaning this up, we should probably have "FeatureARM", for CPUs which support ARM-mode execution, rather than "FeatureNoARM"; negative features are confusing.

That sounds like a good idea. I'll update this patch tomorrow, unless there are any objections.

After looking into that, I am not sure if replacing FeatureNoARM with FeatureARM is a good idea for practical reasons. The advantage of assuming all CPUs/architectures have ARM mode unless they have FeatureNoARM is the following: it allows the architecture to disable ARM mode for CPUs that have ARM mode. For example, currently llc < foo.ll.ll -mtriple=thumbv6m-linux-gnueabi will assume ARM mode is not available (as the v6m architecture does not support ARM mode), even though the generic CPU has ARM mode.

With FeatureARM, the generic CPU would have to have this feature, but when the features of the architecture (v6m) are applied, FeatureARM cannot be removed, so the behavior will be different to the current one (Architecture features are applied after CPU features I think). We could rename FeatureARM to FeatureThumbOnly, which is a bit clearer in my opinion.

[rengolin]

In D35569#816129, @fhahn wrote:

With FeatureARM, the generic CPU would have to have this feature, but when the features of the architecture (v6m) are applied, FeatureARM cannot be removed, so the behavior will be different to the current one (Architecture features are applied after CPU features I think). We could rename FeatureARM to FeatureThumbOnly, which is a bit clearer in my opinion.

Correct. FeatureNoARM is specific to M-class. I don't mind how we call it as long as it's explicit the intention, and I think both FeatureNoARM and FeatureThumbOnly are clear.

About the error message, this is a similar problem than D34875: inconsistent states generate the wrong behaviour. But what can we realistically do about it depends on where we're coming from.

For example, if there's an arm attribute forcing a function to emit ARM in a Thumb-only core, do we do it? If another attribute forces VFP PCS, do we do it? The answer is: it depends.

If the user is trying to pass "-target armv6m -marm" then, well, the user is wrong and Clang/llc/lli should tell them that.

If the user has an attribute or inline assembly with a directive, then probably the user knows what is needed, and probably has done that on purpose, so, should we deny them of their request?

An example of that not being wrong is the unwinder. We have added .arch and .fpu directives on functions that unwind registers that are only valid on those sub-architectures, and we guarantee that they will never be called on targets that do not support it. We must have a single unwinder that works on all sub-targets, in the same way we want one kernel to run on all sub-architectures by default, etc.

So, the error message conundrum is somewhat simple to solve:

• If they come from flags, make sure they're sane and error out early if not, in the front-end
• If they come from user input (attribute/directive/inline asm), then just do whatever the user told you to do
• Everything else should be an error

In the second case above, if we generate what the user asks, and they come to use crying, well, you can easily tell them: it's your own fault.

But if we try to be smart and don't let the user to do what they explicitly requested, they'll be "smarter" and do horrible things like linker script magic or binary live patching.

The errors are hand-coded IR, other front-ends bad lowering, out-of-tree passes, bugs in the middle/back-end. Those we can emit errors for, but only if we're sure there are no attributes involved (can we?).

If we can reliably tell what's in the first and second categories, we should at least be able to create some tests and errors in the right places.

cheers,
--renato

test/CodeGen/ARM/scavenging.mir

2

I expect "arm" triple to fail here with an error message like "sorry, no ARM support".

If that's what happen now, then we should have a negative test, too.

[fhahn]

Thanks Renato for taking the time to provide such a detailed response. The case is not as clear cut as I initially thought. I'll have to think about it a bit more.

In the meantime, I created D35826, which adds a error message to Clang's driver if -marm/-mno-thumb is used in combination with M-profile -mcpu/-march options, as it seems there's agreement that that's a thing we should definitely check in the frontend. I'll also plan to follow this up with a patch that errors/warns if the -mcpu/-march options are incompatible.

[fhahn]

I checked with the authors of the ACLE to clarify the intended reading of the relevant bit from section 7.3 ("The implementation must generate code in the required state unless it is impossible to do so.").
In the case where __attribute__((target("arm"))) is used for Thumb-only targets, there should be an error, as it is impossible to generate ARM code for the selected target and the compiler would not know which target to fall back to to generate ARM code.

GCC conforms to this interpretation of the spec. Also, llvm-mc will reject assembler using .arm directives for Thumb-ony targets (test/MC/ARM/arm-thumb-cpus.s), which seems inconsistent with llc's current behavior.

I think it should be an error in llc too, as llc would have to know which target with ARM support it should fall back to. For "union binaries", people would have to make them at link time, similar to GCC.

What do you think?

[fhahn]

ping. @rengolin any more thoughts?

[rengolin]

Sorry, long holidays... :)

I agree with your interpretation of the ACLE and with the fact that llvm-mc and llc should behave accordingly.

Can you add a simple test to see that we do get the error when selecting -mtriple=arm -mcpu=m0?

With the added negative test, LGTM. Thanks!

cheers,
--renato

[fhahn]

test/CodeGen/ARM/scavenging.mir

2

Unfortunately that case isn't covered by this patch and D35627. Those patches only address the issue with -thumb-mode in the target features.

After thinking about it for a bit I am not sure we can check the case above unless we are fine breaking cases where the default target triple is arm-....., e.g. that llc -mcpu=cortex-m0 foo.ll will fail if the default target triple is an "arm" triple.

[richard.barton.arm]

Agreed with what Florian says. My understanding of -target is that it is used to set up which flavour of toolchain we are using and using it to set the architecture is a shorthand for saying '-target=arm... -march=armvX'. I also expect it to be somewhat like GCC where the 'arm-...' triples cover ARM/AArch32 architecture in general and not the ARM ISA per se.

So I would expect -target, -march and -mcpu to set the architecture and override eachother in that order, i.e mcpu > march > target, then -marm|-mthumb set the ISA. We warn if the ISA is incompatible with the architecture.

[fhahn]

I think this change should still go in (in combination with D35627) to get proper error messages in the attribute case. Please let me know if there are any more concerns.

[echristo]

In D35569#816289, @rengolin wrote:

In D35569#816129, @fhahn wrote:

With FeatureARM, the generic CPU would have to have this feature, but when the features of the architecture (v6m) are applied, FeatureARM cannot be removed, so the behavior will be different to the current one (Architecture features are applied after CPU features I think). We could rename FeatureARM to FeatureThumbOnly, which is a bit clearer in my opinion.

Correct. FeatureNoARM is specific to M-class. I don't mind how we call it as long as it's explicit the intention, and I think both FeatureNoARM and FeatureThumbOnly are clear.

Well, clear or not is a bit subjective ;)

That said, I think this is a strict improvement.

-eric

[fhahn]

In D35569#836330, @echristo wrote:

In D35569#816289, @rengolin wrote:

Correct. FeatureNoARM is specific to M-class. I don't mind how we call it as long as it's explicit the intention, and I think both FeatureNoARM and FeatureThumbOnly are clear.

Well, clear or not is a bit subjective ;)

Agreed, I think there's no need to change that.

That said, I think this is a strict improvement.

Great, thanks for having another look. I'll commit the change soon soon.

lib/Target/ARM/ARM.td

 def FeatureMClass : SubtargetFeature<"mclass", "ARMProcClass", "MClass",
                                      "Is microcontroller profile ('M' series)">;
 
 
 def FeatureThumb2 : SubtargetFeature<"thumb2", "HasThumb2", "true",
                                      "Enable Thumb2 instructions">;
 
 def FeatureNoARM  : SubtargetFeature<"noarm", "NoARM", "true",
-                                     "Does not support ARM mode execution",
-                                     [ModeThumb]>;
-
+                                     "Does not support ARM mode execution">;
 
 //===----------------------------------------------------------------------===//
 // ARM ISAa.
 //
 
 def HasV4TOps   : SubtargetFeature<"v4t", "HasV4TOps", "true",
                                    "Support ARM v4T instructions">;
 

 
 def ARMv6k    : Architecture<"armv6k",    "ARMv6k",   [HasV6KOps]>;
 
 def ARMv6kz   : Architecture<"armv6kz",   "ARMv6kz",  [HasV6KOps,
                                                        FeatureTrustZone]>;
 
 def ARMv6m    : Architecture<"armv6-m",   "ARMv6m",   [HasV6MOps,
                                                        FeatureNoARM,
+                                                       ModeThumb,
                                                        FeatureDB,
                                                        FeatureMClass]>;
 
 def ARMv6sm   : Architecture<"armv6s-m",  "ARMv6sm",  [HasV6MOps,
                                                        FeatureNoARM,
+                                                       ModeThumb,
                                                        FeatureDB,
                                                        FeatureMClass]>;
 
 def ARMv7a    : Architecture<"armv7-a",   "ARMv7a",   [HasV7Ops,
                                                        FeatureNEON,
                                                        FeatureDB,
                                                        FeatureDSP,
                                                        FeatureAClass]>;
def ARMv7r    : Architecture<"armv7-r",   "ARMv7r",   [HasV7Ops,
                                                        FeatureDB,
                                                        FeatureDSP,
                                                        FeatureHWDivThumb,
                                                        FeatureRClass]>;
 
 def ARMv7m    : Architecture<"armv7-m",   "ARMv7m",   [HasV7Ops,
                                                        FeatureThumb2,
                                                        FeatureNoARM,
+                                                       ModeThumb,
                                                        FeatureDB,
                                                        FeatureHWDivThumb,
                                                        FeatureMClass]>;
 
 def ARMv7em   : Architecture<"armv7e-m",  "ARMv7em",  [HasV7Ops,
                                                        FeatureThumb2,
                                                        FeatureNoARM,
+                                                       ModeThumb,
                                                        FeatureDB,
                                                        FeatureHWDivThumb,
                                                        FeatureMClass,
                                                        FeatureDSP]>;
 
 def ARMv8a    : Architecture<"armv8-a",   "ARMv8a",   [HasV8Ops,
                                                        FeatureAClass,
                                                        FeatureDB,
def ARMv8r    : Architecture<"armv8-r",   "ARMv8r",   [HasV8Ops,
                                                        FeatureMP,
                                                        FeatureVirtualization,
                                                        FeatureFPARMv8,
                                                        FeatureNEON]>;
 
 def ARMv8mBaseline : Architecture<"armv8-m.base", "ARMv8mBaseline",
                                                       [HasV8MBaselineOps,
                                                        FeatureNoARM,
+                                                       ModeThumb,
                                                        FeatureDB,
                                                        FeatureHWDivThumb,
                                                        FeatureV7Clrex,
                                                        Feature8MSecExt,
                                                        FeatureAcquireRelease,
                                                        FeatureMClass]>;
 
 def ARMv8mMainline : Architecture<"armv8-m.main", "ARMv8mMainline",
                                                       [HasV8MMainlineOps,
                                                        FeatureNoARM,
+                                                       ModeThumb,
                                                        FeatureDB,
                                                        FeatureHWDivThumb,
                                                        Feature8MSecExt,
                                                        FeatureAcquireRelease,
                                                        FeatureMClass]>;
 
 // Aliases
 def IWMMXT   : Architecture<"iwmmxt",      "ARMv5te",  [ARMv5te]>;

test/CodeGen/ARM/scavenging.mir

-# RUN: llc -o - %s -mtriple=arm-arm-none-eabi -mcpu=cortex-m0 -run-pass scavenger-test | FileCheck %s
+# RUN: llc -o - %s -mtriple=thumb-arm-none-eabi -mcpu=cortex-m0 -run-pass scavenger-test | FileCheck %s
 ---
 # CHECK-LABEL: name: scavengebug0
 # Make sure we are not spilling/using a physreg used in the very last
 # instruction of the scavenging range.
 # CHECK-NOT: tSTRi {{.*}}%r0,{{.*}}%r0
 # CHECK-NOT: tSTRi {{.*}}%r1,{{.*}}%r1
 # CHECK-NOT: tSTRi {{.*}}%r2,{{.*}}%r2
 # CHECK-NOT: tSTRi {{.*}}%r3,{{.*}}%r3
 # CHECK-NOT: tSTRi {{.*}}%r4,{{.*}}%r4