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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

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



================
Comment at: lib/Linker/IRMover.cpp:1297
+    std::string SrcModuleInlineAsm = SrcM->getModuleInlineAsm();
+    // Add directives to support mixing module-level inline assembly from ARM
+    // and Thumb modules.
----------------
Suggest outlining this handling in a helper function that takes the original module inline asm string and passes back a new one.


================
Comment at: lib/Linker/IRMover.cpp:1299
+    // and Thumb modules.
+    if (SrcTriple.getArch() == Triple::thumb ||
+        SrcTriple.getArch() == Triple::thumbeb)
----------------
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? 


================
Comment at: test/MC/ARM/inline-asm-switch-mode.ll:10
+
+; CHECK: .code 16
+; CHECK-next: .p2align 1
----------------
Confused as to what is being tested here since no llvm-link invocation.


https://reviews.llvm.org/D34622