[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 ↗

(On Diff #99317)

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

Formatting seems off - can you run clang-format?

1486

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

[fhahn]

added missing triples, fixed formatting

lib/Support/Triple.cpp

1486

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/Support/Triple.cpp

bool Triple::isLittleEndian() const {
   case Triple::renderscript64:
     return true;
   default:
     return false;
   }
 }
 
 bool Triple::isCompatibleWith(const Triple &Other) const {
+  // ARM and Thumb triples are compatible, if subarch, vendor and OS match.
+  if ((getArch() == Triple::thumb && Other.getArch() == Triple::arm) ||
+      (getArch() == Triple::arm && Other.getArch() == Triple::thumb) ||
+      (getArch() == Triple::thumbeb && Other.getArch() == Triple::armeb) ||
+      (getArch() == Triple::armeb && Other.getArch() == Triple::thumbeb))
+    if (getVendor() == Triple::Apple)
+      return getSubArch() == Other.getSubArch() &&
+             getVendor() == Other.getVendor() && getOS() == Other.getOS();
+    else
+      return getSubArch() == Other.getSubArch() &&
+             getVendor() == Other.getVendor() && getOS() == Other.getOS() &&
+             getEnvironment() == Other.getEnvironment() &&
+             getObjectFormat() == Other.getObjectFormat();
+
   // If vendor is apple, ignore the version number.
   if (getVendor() == Triple::Apple)
     return getArch() == Other.getArch() && getSubArch() == Other.getSubArch() &&
            getVendor() == Other.getVendor() && getOS() == Other.getOS();
 
   return *this == Other;
 }
 

test/LTO/ARM/Inputs/thumb.ll

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

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

+; Testcase to check that functions from a Thumb module can be inlined in an
+; ARM function.
+;
+; 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 2> %t3.out| FileCheck %s
+; RUN: FileCheck --allow-empty --input-file %t3.out --check-prefix STDERR %s
+
+target triple = "armv7-linux-gnueabihf"
+
+; CHECK: .code  32
+; CHECK-NEXT: main
+; CHECK-NEXT: .fnstart
+; CHECK-NEXT: mov r0, #30
+
+; CHECK: .code  16
+; CHECK-NEXT: .thumb_func
+; 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
+}
+
+; STDERR-NOT: warning: Linking two modules of different target triples:

test/Linker/Inputs/thumb.ll

+target triple = "thumbv7-linux-gnueabihf"
+
+define i32 @foo(i32 %a, i32 %b) #0 {
+entry:
+  %add = add i32 %a, %b
+  ret i32 %add
+}
+
+define i32 @bar(i32 %a, i32 %b) #1 {
+entry:
+  %add = add i32 %a, %b
+  ret i32 %add
+}
+
+attributes #0 = { "target-features"="-thumb-mode" }
+attributes #1 = { "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 2> %t3.out | llc | FileCheck %s
+; RUN: FileCheck --allow-empty --input-file %t3.out --check-prefix STDERR %s
+
+target triple = "armv7-linux-gnueabihf"
+
+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 @ @main
+; CHECK-NEXT: main
+
+; CHECK: .code  32 @ @foo
+; CHECK-NEXT: foo
+
+; CHECK: .code  16 @ @bar
+; CHECK-NEXT: .thumb_func
+; CHECK-NEXT: bar
+
+; STDERR-NOT: warning: Linking two modules of different target triples: