[Linker] Remove warning when linking ARM and Thumb IR modules.

[fhahn]

Summary

This patch updates Triple::isCompatibleWith to make armxx and thumbxx
triples compatible, as long as the subarch, vendor, os, envorionment and
object format match. Thumb/ARM code generation should be controlled
using the thumb-mode per-function target feature rather than by the
triple to allow mixing Thumb and ARM functions.

D33448 updates Clang's codegen to add thumb-mode for all functions with
armxx or thumbxx triples.

[echristo]

I'm not even really sure that +thumb_mode works in all cases. Why not just have a single TargetMachine at that rate for the ARM back end?

[tejohnson]

lib/Linker/IRMover.cpp

1275

If we're going to do this in the IRMover, it would probably be more efficient to do this once over the whole source module, not just the symbols being linked in one by one, so that the checking etc only needs to be done once per module. Just walk over all the src module functions and apply the attribute.

I guess this is needed because it looks like thumb-mode is typically added in the MC time, but that is too late since these modules are already combined, right?

Is there a drawback to always adding the +/-thumb-mode attribute to thumb/arm modules earlier, like during clang CodeGen?

[fhahn]

@echristo I've created a separate review to remove the Thumb target machines (D33318)

Are there any cases in particular you are worried about when it comes to Thumb/ARM code generation in the same compilation unit? I was thinking about compiling and running a big set of tests with a version of clang that randomly adds +thumb-mode to some functions. This should help increasing the confidence in mixing Thumb/ARM code generation in the same compilation unit.

@tejohnson as far as I am aware of, one (probably minor) drawback of doing it in clang would be that other frontends might also need to add +-thumb-mode in case they want to link ARM and Thumb modules. Also, when it comes to the warning about incompatible triples from the linker, we would have to trust clang to add the +thumb-mode feature in order to silence the message for armxx and thumbxx triples.

[fhahn]

@echristo I've uncovered a bug causing BL being used to call internal ARM functions from Thumb functions and have put up a patch for that problem: D33436

With this patch, multiple runs of the LNT test suite pass built for ARM using a version of clang that randomly adds +thumb-mode to functions.

[fhahn]

Rebased and removed code that adds thumb-mode from the linker, as the thumb-mode feature should be added at an earlier stage.

This patch now only removes the warning when linking ARM and Thumb IR modules.

[tejohnson]

lib/Support/Triple.cpp

1482 ↗

(On Diff #100054)

Formatting seems off - can you run clang-format?

1486 ↗

(On Diff #100054)

What happens if the above 2 checks are run for Apple?

[fhahn]

added missing triples, fixed formatting

lib/Support/Triple.cpp

1486 ↗

(On Diff #100054)

isCompatibleWith treats apple triples in a special way and I tried to match the check below for armxx and thumbxx triples

[tejohnson]

LGTM

Probably only need the llvm-link test though.

[fhahn]

I think the LTO test is slightly different from the linker test, it ensures that a function from a Thumb module can be inlined in a ARM function and that ARM code is generated for that function. I'll remove it if you think it does not add value to the test suite.

[tejohnson]

In D33287#765431, @fhahn wrote:

I think the LTO test is slightly different from the linker test, it ensures that a function from a Thumb module can be inlined in a ARM function and that ARM code is generated for that function.

Ah ok, that does seem valuable. Please add a comment to that effect to the test.

[fhahn]

Added comment to LTO test case.

[fhahn]

I'll hold off merging this patch until D33436 lands, which fixes a problem with mixed ARM/Thumb codegen

lib/Linker/IRMover.cpp

 
 // This function returns true if the triples match.
 static bool triplesMatch(const Triple &T0, const Triple &T1) {
   // If vendor is apple, ignore the version number.
   if (T0.getVendor() == Triple::Apple)
     return T0.getArch() == T1.getArch() && T0.getSubArch() == T1.getSubArch() &&
            T0.getVendor() == T1.getVendor() && T0.getOS() == T1.getOS();
 
+  // Allow linking ARM and Thumb modules together without warning, if subarch,
+  // vendor and OS match.
+  if ((T0.getArch() == Triple::thumb && T1.getArch() == Triple::arm) ||
+      (T0.getArch() == Triple::arm && T1.getArch() == Triple::thumb))
+    return T0.getSubArch() == T1.getSubArch() &&
+           T0.getVendor() == T1.getVendor() && T0.getOS() == T1.getOS();
+
   return T0 == T1;
 }
 
 // This function returns the merged triple.
 static std::string mergeTriples(const Triple &SrcTriple,
                                 const Triple &DstTriple) {
   // If vendor is apple, pick the triple with the larger version number.
   if (SrcTriple.getVendor() == Triple::Apple)
     if (DstTriple.isOSVersionLT(SrcTriple))
       return SrcTriple.str();
 
   return DstTriple.str();
 }
 
+// Add thumb-mode to target-features if we link ARM and Thumb code together.
+void maybeAddARMSubtargetFeatures(const Triple &SrcTriple,
+                                  const Triple &DstTriple,
+                                  GlobalValue *GV) {
+  // Only consider triples with Arm or Thumb architecture.
+  if (SrcTriple.getArch() == DstTriple.getArch())
+    return;
+  if (SrcTriple.getArch() != Triple::thumb && SrcTriple.getArch() != Triple::arm)
+    return;
+  if (DstTriple.getArch() != Triple::thumb && DstTriple.getArch() != Triple::arm)
+    return;
+
+  const std::string thumbMode = SrcTriple.getArch() == Triple::arm ?
+                                  "-thumb-mode" : "+thumb-mode";
+
+  llvm::Function *Fn = dyn_cast<llvm::Function>(GV);
+  if (!Fn)
+    return;
+
+  Attribute targetFeatures;
+  if (Fn->hasFnAttribute("target-features")) {
+    targetFeatures = Fn->getFnAttribute("target-features");
+  }
+
+  std::string newFeatures = targetFeatures.getValueAsString();
+  // Do not add thumb-mode if it is already part of the target-features.
+  if (newFeatures.find("thumb-mode") != std::string::npos)
+    return;
+
+  if (newFeatures.size() > 0)
+    newFeatures += ",";
+
+  newFeatures += thumbMode;
+  Fn->addFnAttr("target-features", newFeatures);
+}
+
 Error IRLinker::run() {
   // Ensure metadata materialized before value mapping.
   if (SrcM->getMaterializer())
     if (Error Err = SrcM->getMaterializer()->materializeMetadata())
       return Err;
 
   // Inherit the target data from the source module if the destination module
   // doesn't have one already.
Error IRLinker::run() {
   // Loop over all of the linked values to compute type mappings.
   computeTypeMapping();
 
   std::reverse(Worklist.begin(), Worklist.end());
   while (!Worklist.empty()) {
     GlobalValue *GV = Worklist.back();
     Worklist.pop_back();
 
+    maybeAddARMSubtargetFeatures(SrcTriple, DstTriple, GV);
+
     // Already mapped.
     if (ValueMap.find(GV) != ValueMap.end() ||
         AliasValueMap.find(GV) != AliasValueMap.end())
       continue;
 
     assert(!GV->isDeclaration());
     Mapper.mapValue(*GV);
     if (FoundError)

test/LTO/ARM/Inputs/thumb.ll

+target triple = "thumbv7-apple-ios"
+
+define i32 @foo(i32 %a, i32 %b) {
+entry:
+  %add = add i32 %a, %b
+  ret i32 %add
+}
+
+define i32 @bar(i32 %a, i32 %b) {
+entry:
+  %add = add i32 %a, %b
+  ret i32 %add
+}

test/LTO/ARM/link-arm-and-thumb.ll

+; RUN: llvm-as %s -o %t1.bc
+; RUN: llvm-as %p/Inputs/thumb.ll -o %t2.bc
+; RUN: llvm-lto -exported-symbol main \
+; RUN:          -exported-symbol bar \
+; RUN:          -filetype=asm \
+; RUN:          -o - \
+; RUN:          %t1.bc %t2.bc | FileCheck %s
+
+target triple = "armv7-apple-ios"
+
+; CHECK: .code  32
+; CHECK-NEXT: _main
+; CHECK-NEXT: mov  r0, #30
+
+; CHECK: .code  16
+; CHECK-NEXT: .thumb_func   _bar
+; CHECK-NEXT: _bar
+
+declare i32 @foo(i32 %a, i32 %b);
+
+define i32 @main() {
+entry:
+  %add = call i32 @foo(i32 10, i32 20)
+  ret i32 %add
+}

test/Linker/Inputs/thumb.ll

+target triple = "thumbv7-apple-ios"
+
+define i32 @foo(i32 %a, i32 %b) #0 {
+entry:
+  %add = add i32 %a, %b
+  ret i32 %add
+}
+
+define i32 @bar(i32 %a, i32 %b) {
+entry:
+  %add = add i32 %a, %b
+  ret i32 %add
+}
+
+attributes #0 = { "target-features"="-thumb-mode" }

test/Linker/link-arm-and-thumb.ll

+; RUN: llvm-as %s -o %t1.bc
+; RUN: llvm-as %p/Inputs/thumb.ll -o %t2.bc
+; RUN: llvm-link %t1.bc %t2.bc -S | llc | FileCheck %s
+
+target triple = "armv7-apple-ios"
+
+declare i32 @foo(i32 %a, i32 %b);
+
+define i32 @main() {
+entry:
+  %add = call i32 @foo(i32 10, i32 20)
+  ret i32 %add
+}
+
+; CHECK: .code  32
+; CHECK-NEXT: _main
+
+; CHECK: .code  32
+; CHECK-NEXT: _foo
+
+; CHECK: .code  16
+; CHECK-NEXT: .thumb_func   _bar
+; CHECK-NEXT: _bar