[Bitcode] Add thumb-mode to target-features in metadata loader.

[fhahn]

Summary

The thumb-mode target feature is used to control mixed ARM/Thumb
code generation in a single compilation unit. D33287 adds thumb-mode to
target-features for arm/thumb triples.

David Blaikie also suggested that we might be able to get rid of the
thumb triples altogether after the bitcode reader. Currently there are
about 30 checks that use Triple::thumb/thumbeb in the LLVM source code.

I had a look at most of them and unfortunately it seems like replacing
most of them with ARMSubtarget::isThumb will be quite hard, because
either no TargetMachine instance (e.g. the uses in
lib/Transforms/IPO/LowerTypeTests.cpp) or no function is available (e.g.
when initializing global sections in lib/MC/MCObjectFileInfo.cpp or
printing model-level inline asm lib/Target/ARM/ARMAsmPrinter.cpp).

I would appreciate any thoughts on Triple::thumb uses. At the moment it
seems to me replacing the remaining uses requires quite a bit of work
for relatively little benefits.

[javed.absar]

lib/Bitcode/Reader/MetadataLoader.cpp

594

maybe you should add isARMorThumb to ADT/Triple.h and use it, much like isPS4(), isAndroid etc?

test/Bitcode/thumb-mode-upgrade-arm.ll

1

Could the same check be achieved using something like 'llvm-as < %s | llvm-dis ..."?

[dblaikie]

lib/Bitcode/Reader/MetadataLoader.cpp

599–600

Probably cheaper to get the attribute & test if it succeeded, than to test then get separately (one lookup rather than two). I'd check other API uses to see if there's a good idiom for a single query+test sort of thing as it's not readily apparent to me from the API (I'd expect Attribute to have an explicit operator bool, but it doesn't look like it has one)

601

Is this how other target features are tested for in other parts of the codebase? I'd be worried that 'thumb-mode' could appear as a substring of a target feature and cause this to have a false positive

601

So is the idea that all new bitcode for ARM targets will have explicit -thumb-mode or +thumb-mode, so you'll know when upgrading that you're not overriding a default (eg: if you see a thumb triple but none of the functions have thumb-mode that doesn't mean each function was attributed with "not thumb")?

[pcc]

either no TargetMachine instance (e.g. the uses in
lib/Transforms/IPO/LowerTypeTests.cpp)

I believe that the jump table entries created by this pass will want to have the same thumbness as the referenced function, but @eugenis may know more.

[eugenis]

In D34028#776424, @pcc wrote:

either no TargetMachine instance (e.g. the uses in
lib/Transforms/IPO/LowerTypeTests.cpp)

I believe that the jump table entries created by this pass will want to have the same thumbness as the referenced function, but @eugenis may know more.

Right. And ARM for external functions (the same as PLT).

[fhahn]

Address comments.

@ppc @eugenis it sounds like we should use the function target-features when determining the thumbness of the jump table entries, otherwise this could probably confuse the linker for mixed ARM/Thumb. codegen. I'll look into that!

lib/Bitcode/Reader/MetadataLoader.cpp

594

Maybe it would make sense to add an isThumb() and isARm() method to Triple. In particular, isThumb could be used at a couple of places. But ideally those checks should be replaced to use the ARMSubtargetFeatures.

599–600

There is no need to check if the feature is present, getFnAttribute returns an empty attribute in case the attribute does not exist.

601

No, AFAIK target features are only checked during CodeGen, using a TargetMachine or Subtarget instance. I think initializing an ARMSubtarget instance in this case would be overkill. I've strengthened the check though, so it should not match if thumb-mode appears in a substring.

Re your second comment: yes the idea is that code generated by clang now sets +/-thumb-mode for all functions. For functions without a thumb-mode target feature and a thumb triple, Thumb code should be generated, thus +thumb-mode is added.

test/Bitcode/thumb-mode-upgrade-arm.ll

1

I think the reason to include a bitcode file is to make sure older versions of the bitcode format are handled correctly going forward

[mehdi_amini]

lib/Bitcode/Reader/MetadataLoader.cpp

594

This will be done for each and every function in the module, while this check could be done at the module level once.
I kind of remember that building a Triple from a string isn't "free", but possible this is negligible in face of the cost of deserializing bitcode.

[fhahn]

lib/Bitcode/Reader/MetadataLoader.cpp

594

@mehdi_amini I've updated the patch to only instantiate Triple once.

[fhahn]

lib/Bitcode/Reader/MetadataLoader.cpp

594

@javed.absar I've opened D34682 which adds isThumb and isARM functions to the Triple class.

[mehdi_amini]

Should this be documented in the LLVM LangRef? (or elsewhere?)
(Otherwise I'd expect some documentation on the `upgradeThumbMode``` function at least)

[mehdi_amini]

(maybe it is already?)

[fhahn]

Sorry for the long delay! I am dropping this now, as I think it's not applicable or needed any more. Thanks for having a look

lib/Bitcode/Reader/BitcodeReader.cpp

Error BitcodeReader::globalCleanup() {
   // Patch the initializers for globals and aliases up.
   if (Error Err = resolveGlobalAndIndirectSymbolInits())
     return Err;
   if (!GlobalInits.empty() || !IndirectSymbolInits.empty())
     return error("Malformed global initializer set");
 
   // Look for intrinsic functions which need to be upgraded at some point
   for (Function &F : *TheModule) {
+    MDLoader->upgradeThumbMode(F);
     MDLoader->upgradeDebugIntrinsics(F);
     Function *NewFn;
     if (UpgradeIntrinsicFunction(&F, NewFn))
       UpgradedIntrinsics[&F] = NewFn;
     else if (auto Remangled = Intrinsic::remangleIntrinsicFunction(&F))
       // Some types could be renamed during loading if several modules are
       // loaded in the same LLVMContext (LTO scenario). In this case we should
       // remangle intrinsics names as well.

lib/Bitcode/Reader/MetadataLoader.h

public:
   /// Parse a `METADATA_KIND` block for the current module.
   Error parseMetadataKinds();
 
   unsigned size() const;
   void shrinkTo(unsigned N);
 
   /// Perform bitcode upgrades on llvm.dbg.* calls.
   void upgradeDebugIntrinsics(Function &F);
+
+  /// Add thumb-mode target-feature for arm and thumb triples.
+  void upgradeThumbMode(Function &F);
 };
 }
 
 #endif // LLVM_LIB_BITCODE_READER_METADATALOADER_H

lib/Bitcode/Reader/MetadataLoader.cpp

public:
   Error parseMetadataKinds();
 
   void setStripTBAA(bool Value) { StripTBAA = Value; }
   bool isStrippingTBAA() { return StripTBAA; }
 
   unsigned size() const { return MetadataList.size(); }
   void shrinkTo(unsigned N) { MetadataList.shrinkTo(N); }
   void upgradeDebugIntrinsics(Function &F) { upgradeDeclareExpressions(F); }
+
+  void upgradeThumbMode(Function &F) {
+    const Triple TT(TheModule.getTargetTriple());
+    if (TT.getArch() != Triple::thumb && TT.getArch() != Triple::thumbeb &&
+        TT.getArch() != Triple::arm && TT.getArch() != Triple::armeb)
+      return;
+
+    std::string TargetFeatures;
+    if (F.hasFnAttribute("target-features"))
+      TargetFeatures = F.getFnAttribute("target-features").getValueAsString();
+    if (TargetFeatures.find("thumb-mode") != std::string::npos)
+      return;
+
+    bool isArm = TT.getArch() == Triple::arm || TT.getArch() == Triple::armeb;
+    TargetFeatures += TargetFeatures.empty() ? "" : ",";
+    TargetFeatures += isArm ? "-" : "+";
+    TargetFeatures += "thumb-mode";
+    F.addFnAttr("target-features", TargetFeatures);
+  }
 };
 
 static Error error(const Twine &Message) {
   return make_error<StringError>(
       Message, make_error_code(BitcodeError::CorruptedBitcode));
 }
 
 Expected<bool>

 bool MetadataLoader::isStrippingTBAA() { return Pimpl->isStrippingTBAA(); }
 
 unsigned MetadataLoader::size() const { return Pimpl->size(); }
 void MetadataLoader::shrinkTo(unsigned N) { return Pimpl->shrinkTo(N); }
 
 void MetadataLoader::upgradeDebugIntrinsics(Function &F) {
   return Pimpl->upgradeDebugIntrinsics(F);
 }
+
+void MetadataLoader::upgradeThumbMode(Function &F) {
+  return Pimpl->upgradeThumbMode(F);
+}

test/Bitcode/thumb-mode-upgrade-arm.ll

+; RUN: llvm-dis -o - %s.bc | FileCheck %s
+
+target triple = "arm-apple-ios"
+
+define void @foo() #0 {
+  ret void
+}
+define void @bar() {
+  ret void
+}
+
+attributes #0 = { "target-features"="+thumb-mode" }
+
+; CHECK: void @foo() [[ThumbModeAttr:#[0-7]]]
+; CHECK: void @bar() [[NoThumbModeAttr:#[0-7]]]
+
+; CHECK: attributes [[ThumbModeAttr]] = { "target-features"="+thumb-mode" }
+; CHECK: attributes [[NoThumbModeAttr]] = { "target-features"="-thumb-mode" }

test/Bitcode/thumb-mode-upgrade-thumb.ll

+; RUN: llvm-dis -o - %s.bc | FileCheck %s
+
+target triple = "thumb-apple-ios"
+
+define void @foo() #0 {
+  ret void
+}
+define void @bar() {
+  ret void
+}
+
+attributes #0 = { "target-features"="-thumb-mode" }
+
+; CHECK: void @foo() [[NoThumbModeAttr:#[0-7]]]
+; CHECK: void @bar() [[ThumbModeAttr:#[0-7]]]
+
+; CHECK: attributes [[NoThumbModeAttr]] = { "target-features"="-thumb-mode" }
+; CHECK: attributes [[ThumbModeAttr]] = { "target-features"="+thumb-mode" }