[Linker] Add directives to support mixing ARM/Thumb module-level inline asm.

[fhahn]

Summary

By prepending .text .thumb .balign 2 to the module-level inline
assembly from a Thumb module, the assembler will generate the assembly
from that module as Thumb, even if the destination module uses an ARM
triple. Similar directives are used for module-level inline assembly in
ARM modules.

The alignment and instruction set are reset based on the target triple
before emitting the first function label.

[tejohnson]

Can you add comments to the top of your tests as to what is being tested?

lib/Linker/IRMover.cpp

1309

Suggest outlining this handling in a helper function that takes the original module inline asm string and passes back a new one.

1311

Should this first check if the dest module is of the other arch before emitting these?

I guess by always emitting then you don't have to worry about needing to switch the arch again back to the dest arch. I.e. in the case where the dest module is arm, a source module with thumb inline assembly linked in, followed by another source module with arm inline assembly being linked in (I assume this needs to get ".thumb" for source module 1 and then ".arm" for source module 2). Is that why it is being unconditionally emitted?

test/MC/ARM/inline-asm-switch-mode.ll

10 ↗

(On Diff #103950)

Confused as to what is being tested here since no llvm-link invocation.

[fhahn]

Outlined code to add directives, removed MC test case

lib/Linker/IRMover.cpp

1311

Exactly. Also I think there are cases where we have to emit .arm even though we merge an ARM module into an ARM module. For example: We merge module M1 - Thumb and module M2 - ARM into an ARM module.

We have to emit .arm for the inline assembly of M2 (even thought the dest module is also ARM), because M1 set .thumb.

test/MC/ARM/inline-asm-switch-mode.ll

10 ↗

(On Diff #103950)

Ah sorry about that. This test ensures that the mode is reset properly before the first function. I'll remove it from this patch and commit is separately.

[echristo]

Can you give me an example here of what you're trying to accomplish? I'm not sure I understand.

[fhahn]

@echristo when linking ARM and Thumb modules, their module-level inline assembly can contain instructions only available in Thumb or ARM, e.g. ORN in test/Linker/Inputs/thumb-module-inline-asm.ll is only available in Thumb. Without adding .arm/.thumb when joining the inline assembly, assembling destination module will fail, if an ARM triple is used.

[tejohnson]

This looks ok to me now, but please wait for echristo to see if he has more concerns or a different suggestion for handling this situation.

lib/Linker/IRMover.cpp

1311

Ok, that's what I thought - that was the scenario I was trying to describe.

[fhahn]

@echristo Did the case I described make sense?

[fhahn]

Ping.

[echristo]

Not a huge fan, but also don't have any better ideas for now.

All of the work you've been doing seems like it should have a release note - including breaking out this sort of code transformation.

Thanks!

-eric

[fhahn]

Thanks. There are people currently testing mixed ARM/Thumb LTO on real-world projects and I assume there will be a few more bugs that need addressing. I think it would be worth waiting a little bit with adding the release note until they had some more time for testing.

Cheers!

[echristo]

In D34622#806301, @fhahn wrote:

Thanks. There are people currently testing mixed ARM/Thumb LTO on real-world projects and I assume there will be a few more bugs that need addressing. I think it would be worth waiting a little bit with adding the release note until they had some more time for testing.

Cheers!

Sure, as long as something gets written :)

-eric

lib/Linker/IRMover.cpp

for (unsigned I = 0, E = Requirements.size(); I != E; ++I) {
     MDNode *Op = Flags[Flag].first;
     if (!Op || Op->getOperand(2) != ReqValue)
       return stringErr("linking module flags '" + Flag->getString() +
                        "': does not have the required value");
   }
   return Error::success();
 }
 
+/// Return InlineAsm adjusted with target-specific directives if required.
+/// For ARM and Thumb, we have to add directives to select the appropriate ISA
+/// to support mixing module-level inline assembly from ARM and Thumb modules.
+static std::string adjustInlineAsm(const std::string &InlineAsm,
+                                   const Triple &Triple) {
+  if (Triple.getArch() == Triple::thumb || Triple.getArch() == Triple::thumbeb)
+    return ".text\n.balign 2\n.thumb\n" + InlineAsm;
+  if (Triple.getArch() == Triple::arm || Triple.getArch() == Triple::armeb)
+    return ".text\n.balign 4\n.arm\n" + InlineAsm;
+  return InlineAsm;
+}
+
 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.
emitWarning("Linking two modules of different target triples: " +
                 SrcM->getTargetTriple() + "' whereas '" +
                 DstM.getModuleIdentifier() + "' is '" + DstM.getTargetTriple() +
                 "'\n");
 
   DstM.setTargetTriple(SrcTriple.merge(DstTriple));
 
   // Append the module inline asm string.
   if (!IsPerformingImport && !SrcM->getModuleInlineAsm().empty()) {
+    std::string SrcModuleInlineAsm = adjustInlineAsm(SrcM->getModuleInlineAsm(),
+                                                     SrcTriple);
     if (DstM.getModuleInlineAsm().empty())
-      DstM.setModuleInlineAsm(SrcM->getModuleInlineAsm());
+      DstM.setModuleInlineAsm(SrcModuleInlineAsm);
     else
       DstM.setModuleInlineAsm(DstM.getModuleInlineAsm() + "\n" +
-                              SrcM->getModuleInlineAsm());
+                              SrcModuleInlineAsm);
   }
 
   // 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();

test/Linker/Inputs/thumb-module-inline-asm.ll

+target triple = "thumbv7-linux-gnueabihf"
+
+module asm "orn r1, r2, r2"

test/Linker/link-arm-and-thumb-module-inline-asm.ll

+; This test checks that proper directives to switch between ARM and Thumb mode
+; are added when linking ARM and Thumb modules.
+
+; RUN: llvm-as %s -o %t1.bc
+; RUN: llvm-as %p/Inputs/thumb-module-inline-asm.ll -o %t2.bc
+; RUN: llvm-link %t1.bc %t2.bc -S 2> %t3.out | FileCheck %s
+
+target triple = "armv7-linux-gnueabihf"
+
+module asm "add r1, r2, r2"
+
+; CHECK: .text
+; CHECK-NEXT: .balign 4
+; CHECK-NEXT: .arm
+; CHECK-NEXT: add r1, r2, r2
+; CHECK-NEXT: module asm
+; CHECK-NEXT: .text
+; CHECK-NEXT: .balign 2
+; CHECK-NEXT: .thumb
+; CHECK-NEXT: orn r1, r2, r2