[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

683

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

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

2

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

[dblaikie]

lib/Bitcode/Reader/MetadataLoader.cpp

688–689

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)

690

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

690

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

683

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.

688–689

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

690

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

2

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

683

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

683

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

[fhahn]

lib/Bitcode/Reader/MetadataLoader.cpp

683

@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

BitcodeReaderBase::readNameFromStrtab(ArrayRef<uint64_t> Record) {
   if (Record[0] + Record[1] > Strtab.size())
     return {"", {}};
   return {StringRef(Strtab.data() + Record[0], Record[1]), Record.slice(2)};
 }
 
 class BitcodeReader : public BitcodeReaderBase, public GVMaterializer {
   LLVMContext &Context;
   Module *TheModule = nullptr;
+  Triple TargetTriple;
   // Next offset to start scanning for lazy parsing of function bodies.
   uint64_t NextUnreadBit = 0;
   // Last function offset found in the VST.
   uint64_t LastFunctionBlockBit = 0;
   bool SeenValueSymbolTable = false;
   uint64_t VSTOffset = 0;
 
   std::vector<std::string> SectionTable;
private:
 
   Error parseAttributeBlock();
   Error parseAttributeGroupBlock();
   Error parseTypeTable();
   Error parseTypeTableBody();
   Error parseOperandBundleTags();
 
   Expected<Value *> recordValue(SmallVectorImpl<uint64_t> &Record,
-                                unsigned NameIndex, Triple &TT);
+                                unsigned NameIndex);
   void setDeferredFunctionInfo(unsigned FuncBitcodeOffsetDelta, Function *F,
                                ArrayRef<uint64_t> Record);
   Error parseValueSymbolTable(uint64_t Offset = 0);
   Error parseGlobalValueSymbolTable();
   Error parseConstants();
   Error rememberAndSkipFunctionBodies();
   Error rememberAndSkipFunctionBody();
   /// Save the positions of the Metadata blocks and skip parsing the blocks.
std::error_code llvm::errorToErrorCodeAndEmitErrors(LLVMContext &Ctx,
   }
   return std::error_code();
 }
 
 BitcodeReader::BitcodeReader(BitstreamCursor Stream, StringRef Strtab,
                              StringRef ProducerIdentification,
                              LLVMContext &Context)
     : BitcodeReaderBase(std::move(Stream), Strtab), Context(Context),
-      ValueList(Context) {
+      TargetTriple(), ValueList(Context) {
   this->ProducerIdentification = ProducerIdentification;
 }
 
 Error BitcodeReader::materializeForwardReferencedFunctions() {
   if (WillMaterializeAllForwardRefs)
     return Error::success();
 
   // Prevent recursion.
while (true) {
     if (convertToString(Record, 0, BundleTags.back()))
       return error("Invalid record");
     Record.clear();
   }
 }
 
 /// Associate a value with its name from the given index in the provided record.
 Expected<Value *> BitcodeReader::recordValue(SmallVectorImpl<uint64_t> &Record,
-                                             unsigned NameIndex, Triple &TT) {
+                                             unsigned NameIndex) {
   SmallString<128> ValueName;
   if (convertToString(Record, NameIndex, ValueName))
     return error("Invalid record");
   unsigned ValueID = Record[0];
   if (ValueID >= ValueList.size() || !ValueList[ValueID])
     return error("Invalid record");
   Value *V = ValueList[ValueID];
 
   StringRef NameStr(ValueName.data(), ValueName.size());
   if (NameStr.find_first_of(0) != StringRef::npos)
     return error("Invalid value name");
   V->setName(NameStr);
   auto *GO = dyn_cast<GlobalObject>(V);
   if (GO) {
     if (GO->getComdat() == reinterpret_cast<Comdat *>(1)) {
-      if (TT.isOSBinFormatMachO())
+      if (TargetTriple.isOSBinFormatMachO())
         GO->setComdat(nullptr);
       else
         GO->setComdat(TheModule->getOrInsertComdat(V->getName()));
     }
   }
   return V;
 }
 
Error BitcodeReader::parseValueSymbolTable(uint64_t Offset) {
   unsigned FuncBitcodeOffsetDelta =
       Stream.getAbbrevIDWidth() + bitc::BlockIDWidth;
 
   if (Stream.EnterSubBlock(bitc::VALUE_SYMTAB_BLOCK_ID))
     return error("Invalid record");
 
   SmallVector<uint64_t, 64> Record;
 
-  Triple TT(TheModule->getTargetTriple());
-
   // Read all the records for this value table.
   SmallString<128> ValueName;
 
   while (true) {
     BitstreamEntry Entry = Stream.advanceSkippingSubblocks();
 
     switch (Entry.Kind) {
     case BitstreamEntry::SubBlock: // Handled for us already.
while (true) {
     }
 
     // Read a record.
     Record.clear();
     switch (Stream.readRecord(Entry.ID, Record)) {
     default:  // Default behavior: unknown type.
       break;
     case bitc::VST_CODE_ENTRY: {  // VST_CODE_ENTRY: [valueid, namechar x N]
-      Expected<Value *> ValOrErr = recordValue(Record, 1, TT);
+      Expected<Value *> ValOrErr = recordValue(Record, 1);
       if (Error Err = ValOrErr.takeError())
         return Err;
       ValOrErr.get();
       break;
     }
     case bitc::VST_CODE_FNENTRY: {
       // VST_CODE_FNENTRY: [valueid, offset, namechar x N]
-      Expected<Value *> ValOrErr = recordValue(Record, 2, TT);
+      Expected<Value *> ValOrErr = recordValue(Record, 2);
       if (Error Err = ValOrErr.takeError())
         return Err;
       Value *V = ValOrErr.get();
 
       // Ignore function offsets emitted for aliases of functions in older
       // versions of LLVM.
       if (auto *F = dyn_cast<Function>(V))
         setDeferredFunctionInfo(FuncBitcodeOffsetDelta, F, Record);
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.
case bitc::MODULE_CODE_VERSION: {
       UseRelativeIDs = *VersionOrErr >= 1;
       break;
     }
     case bitc::MODULE_CODE_TRIPLE: {  // TRIPLE: [strchr x N]
       std::string S;
       if (convertToString(Record, 0, S))
         return error("Invalid record");
       TheModule->setTargetTriple(S);
+      TargetTriple = Triple(S);
       break;
     }
     case bitc::MODULE_CODE_DATALAYOUT: {  // DATALAYOUT: [strchr x N]
       std::string S;
       if (convertToString(Record, 0, S))
         return error("Invalid record");
       TheModule->setDataLayout(S);
       break;
while (true) {
     }
     Record.clear();
   }
 }
 
 Error BitcodeReader::parseBitcodeInto(Module *M, bool ShouldLazyLoadMetadata,
                                       bool IsImporting) {
   TheModule = M;
-  MDLoader = MetadataLoader(Stream, *M, ValueList, IsImporting,
+  MDLoader = MetadataLoader(Stream, *M, TargetTriple, ValueList, IsImporting,
                             [&](unsigned ID) { return getTypeByID(ID); });
   return parseModule(0, ShouldLazyLoadMetadata);
 }
 
 
 Error BitcodeReader::typeCheckLoadStoreInst(Type *ValType, Type *PtrType) {
   if (!isa<PointerType>(PtrType))
     return error("Load/Store operand is not a pointer type");

lib/Bitcode/Reader/MetadataLoader.h

 // This class handles loading Metadatas.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_LIB_BITCODE_READER_METADATALOADER_H
 #define LLVM_LIB_BITCODE_READER_METADATALOADER_H
 
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/ADT/Triple.h"
 #include "llvm/Support/Error.h"
 
 #include <functional>
 #include <memory>
 
 namespace llvm {
 class BitcodeReaderValueList;
 class BitstreamCursor;

 /// Helper class that handles loading Metadatas and keeping them available.
 class MetadataLoader {
   class MetadataLoaderImpl;
   std::unique_ptr<MetadataLoaderImpl> Pimpl;
   Error parseMetadata(bool ModuleLevel);
 
 public:
   ~MetadataLoader();
-  MetadataLoader(BitstreamCursor &Stream, Module &TheModule,
+  MetadataLoader(BitstreamCursor &Stream, Module &TheModule, Triple &TT,
                  BitcodeReaderValueList &ValueList, bool IsImporting,
                  std::function<Type *(unsigned)> getTypeByID);
   MetadataLoader &operator=(MetadataLoader &&);
   MetadataLoader(MetadataLoader &&);
 
   // Parse a module metadata block
   Error parseModuleMetadata() { return parseMetadata(true); }
 
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

 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/None.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/Triple.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Bitcode/BitcodeReader.h"
 #include "llvm/Bitcode/BitstreamReader.h"
 #include "llvm/Bitcode/LLVMBitCodes.h"
 #include "llvm/IR/Argument.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/AutoUpgrade.h"
 #include "llvm/IR/BasicBlock.h"

 }
 
 class MetadataLoader::MetadataLoaderImpl {
   BitcodeReaderMetadataList MetadataList;
   BitcodeReaderValueList &ValueList;
   BitstreamCursor &Stream;
   LLVMContext &Context;
   Module &TheModule;
+  Triple &TargetTriple;
   std::function<Type *(unsigned)> getTypeByID;
 
   /// Cursor associated with the lazy-loading of Metadata. This is the easy way
   /// to keep around the right "context" (Abbrev list) to be able to jump in
   /// the middle of the metadata block and load any record.
   BitstreamCursor IndexCursor;
 
   /// Index that keeps track of MDString values.
class MetadataLoader::MetadataLoaderImpl {
   }
 
   void upgradeDebugInfo() {
     upgradeCUSubprograms();
     upgradeCUVariables();
   }
 
 public:
-  MetadataLoaderImpl(BitstreamCursor &Stream, Module &TheModule,
+  MetadataLoaderImpl(BitstreamCursor &Stream, Module &TheModule, Triple &TT,
                      BitcodeReaderValueList &ValueList,
                      std::function<Type *(unsigned)> getTypeByID,
                      bool IsImporting)
       : MetadataList(TheModule.getContext()), ValueList(ValueList),
         Stream(Stream), Context(TheModule.getContext()), TheModule(TheModule),
-        getTypeByID(std::move(getTypeByID)), IsImporting(IsImporting) {}
+        TargetTriple(TT), getTypeByID(std::move(getTypeByID)),
+        IsImporting(IsImporting) {}
+
 
   Error parseMetadata(bool ModuleLevel);
 
   bool hasFwdRefs() const { return MetadataList.hasFwdRefs(); }
 
   Metadata *getMetadataFwdRefOrLoad(unsigned ID) {
     if (ID < MDStringRef.size())
       return lazyLoadOneMDString(ID);
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) {
+    bool isArm = TargetTriple.getArch() == Triple::arm ||
+                 TargetTriple.getArch() == Triple::armeb;
+
+    if (TargetTriple.getArch() != Triple::thumb &&
+        TargetTriple.getArch() != Triple::thumbeb && !isArm)
+      return;
+
+    std::string TargetFeatures =
+      F.getFnAttribute("target-features").getValueAsString();
+
+    // Check that the target features do not already contain the thumb-mode
+    // feature.
+    auto Start = TargetFeatures.find("thumb-mode");
+    auto End = Start + std::string("thumb-mode").size();
+    if (Start != std::string::npos &&
+        (TargetFeatures[Start-1] == '-' || TargetFeatures[Start-1] == '+') &&
+        (End == TargetFeatures.size() || TargetFeatures[End] == ','))
+      return;
+
+    TargetFeatures += TargetFeatures.empty() ? "" : ",";
+    TargetFeatures += isArm ? "-" : "+";
+    TargetFeatures += "thumb-mode";
+    F.addFnAttr("target-features", TargetFeatures);
+  }
 };
 
 Expected<bool>
 MetadataLoader::MetadataLoaderImpl::lazyLoadModuleMetadataBlock() {
   IndexCursor = Stream;
   SmallVector<uint64_t, 64> Record;
   // Get the abbrevs, and preload record positions to make them lazy-loadable.
   while (true) {
MetadataLoader &MetadataLoader::operator=(MetadataLoader &&RHS) {
   Pimpl = std::move(RHS.Pimpl);
   return *this;
 }
 MetadataLoader::MetadataLoader(MetadataLoader &&RHS)
     : Pimpl(std::move(RHS.Pimpl)) {}
 
 MetadataLoader::~MetadataLoader() = default;
 MetadataLoader::MetadataLoader(BitstreamCursor &Stream, Module &TheModule,
-                               BitcodeReaderValueList &ValueList,
+                               Triple &TT, BitcodeReaderValueList &ValueList,
                                bool IsImporting,
                                std::function<Type *(unsigned)> getTypeByID)
     : Pimpl(llvm::make_unique<MetadataLoaderImpl>(
-          Stream, TheModule, ValueList, std::move(getTypeByID), IsImporting)) {}
+          Stream, TheModule, TT, ValueList, std::move(getTypeByID),
+          IsImporting)) {}
 
 Error MetadataLoader::parseMetadata(bool ModuleLevel) {
   return Pimpl->parseMetadata(ModuleLevel);
 }
 
 bool MetadataLoader::hasFwdRefs() const { return Pimpl->hasFwdRefs(); }
 
 /// Return the given metadata, creating a replaceable forward reference if

 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 @zar() #1 {
+  ret void
+}
+define void @bar() {
+  ret void
+}
+
+attributes #0 = { "target-features"="+thumb-mode" }
+attributes #1 = { "target-features"="+thumb-mode,+vfp3" }
+
+; CHECK: void @foo() [[ThumbModeAttr1:#[0-7]]]
+; CHECK: void @zar() [[ThumbModeAttr2:#[0-7]]]
+; CHECK: void @bar() [[NoThumbModeAttr:#[0-7]]]
+
+; CHECK: attributes [[ThumbModeAttr1]] = { "target-features"="+thumb-mode" }
+; CHECK: attributes [[ThumbModeAttr2]] = { "target-features"="+thumb-mode,+vfp3" }
+; 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 @zar() #1 {
+  ret void
+}
+define void @bar() {
+  ret void
+}
+
+attributes #0 = { "target-features"="-thumb-mode" }
+attributes #1 = { "target-features"="-thumb-mode,+vfp3" }
+
+; CHECK: void @foo() [[NoThumbModeAttr1:#[0-7]]]
+; CHECK: void @zar() [[NoThumbModeAttr2:#[0-7]]]
+; CHECK: void @bar() [[ThumbModeAttr:#[0-7]]]
+
+; CHECK: attributes [[NoThumbModeAttr1]] = { "target-features"="-thumb-mode" }
+; CHECK: attributes [[NoThumbModeAttr2]] = { "target-features"="-thumb-mode,+vfp3" }
+; CHECK: attributes [[ThumbModeAttr]] = { "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 | 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: define i32 @main() {
+; CHECK: define i32 @main() [[ARM_ATTRS:#[0-9]+]]
 ; CHECK: define i32 @foo(i32 %a, i32 %b) [[ARM_ATTRS:#[0-9]+]]
 ; CHECK: define i32 @bar(i32 %a, i32 %b) [[THUMB_ATTRS:#[0-9]+]]
 
 ; CHECK: attributes [[ARM_ATTRS]] = { "target-features"="-thumb-mode" }
 ; CHECK: attributes [[THUMB_ATTRS]] = { "target-features"="+thumb-mode" }
 
 ; STDERR-NOT: warning: Linking two modules of different target triples: