Fix auto-upgrade of TBAA tags in Bitcode Reader

[mehdi_amini]

Summary

If TBAA is on an intrinsic and it gets upgraded, it'll delete the call
instruction that we collected in a vector. Even if we were to use
WeakVH, it'll drop the TBAA and we'll hit the assert on the upgrade
path.

r263673 gave a shot to make sure the TBAA upgrade happens before
intrinsics upgrade, but failed to account for all cases.

Instead of collecting instructions in a vector, this patch makes it
just upgrade the TBAA on the fly, because metadata are always
already loaded at this point.

[mehdi_amini]

Sorry forgot the test case, will update soon!

[mehdi_amini]

• Fix the assert
• Apply Duncan's suggestion of inlining UpgradeInstWithTBAATag
• Add a llvm-link test

[majnemer]

lib/AsmParser/LLParser.cpp

225–226 ↗

(On Diff #71292)

Range-based loop?

lib/Bitcode/Reader/BitcodeReader.cpp

3752–3753 ↗

(On Diff #71292)

Return after return?

[mehdi_amini]

lib/AsmParser/LLParser.cpp

225–226 ↗

(On Diff #71292)

Sure, good cleanup!
(The whole file deserves some probably)

lib/Bitcode/Reader/BitcodeReader.cpp

3752–3753 ↗

(On Diff #71292)

Some debug slipped...

[mehdi_amini]

Cleanup!

llvm/trunk/include/llvm/IR/AutoUpgrade.h

namespace llvm {
   /// This checks for global variables which should be upgraded. It returns true
   /// if it requires upgrading.
   bool UpgradeGlobalVariable(GlobalVariable *GV);
 
   /// This checks for module flags which should be upgraded. It returns true if
   /// module is modified.
   bool UpgradeModuleFlags(Module &M);
 
-  /// If the TBAA tag for the given instruction uses the scalar TBAA format,
-  /// we upgrade it to the struct-path aware TBAA format.
-  void UpgradeInstWithTBAATag(Instruction *I);
+  /// If the given TBAA tag uses the scalar TBAA format, create a new node
+  /// corresponding to the upgrade to the struct-path aware TBAA format.
+  /// Otherwise return the \p TBAANode itself.
+  MDNode *UpgradeTBAANode(MDNode &TBAANode);
 
   /// This is an auto-upgrade for bitcast between pointers with different
   /// address spaces: the instruction is replaced by a pair ptrtoint+inttoptr.
   Instruction *UpgradeBitCastInst(unsigned Opc, Value *V, Type *DestTy,
                                   Instruction *&Temp);
 
   /// This is an auto-upgrade for bitcast constant expression between pointers
   /// with different address spaces: the instruction is replaced by a pair

llvm/trunk/lib/AsmParser/LLParser.cpp

return Error(ForwardRefMDNodes.begin()->second.second,
                  Twine(ForwardRefMDNodes.begin()->first) + "'");
 
   // Resolve metadata cycles.
   for (auto &N : NumberedMetadata) {
     if (N.second && !N.second->isResolved())
       N.second->resolveCycles();
   }
 
-  for (unsigned I = 0, E = InstsWithTBAATag.size(); I < E; I++)
-    UpgradeInstWithTBAATag(InstsWithTBAATag[I]);
+  for (auto *Inst : InstsWithTBAATag) {
+    MDNode *MD = Inst->getMetadata(LLVMContext::MD_tbaa);
+    assert(MD && "UpgradeInstWithTBAATag should have a TBAA tag");
+    auto *UpgradedMD = UpgradeTBAANode(*MD);
+    if (MD != UpgradedMD)
+      Inst->setMetadata(LLVMContext::MD_tbaa, UpgradedMD);
+  }
 
   // Look for intrinsic functions and CallInst that need to be upgraded
   for (Module::iterator FI = M->begin(), FE = M->end(); FI != FE; )
     UpgradeCallsToIntrinsic(&*FI++); // must be post-increment, as we remove
 
   // 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.

llvm/trunk/lib/Bitcode/Reader/BitcodeReader.cpp

class BitcodeReader : public GVMaterializer {
   SmallVector<Instruction *, 64> InstructionList;
 
   std::vector<std::pair<GlobalVariable*, unsigned> > GlobalInits;
   std::vector<std::pair<GlobalIndirectSymbol*, unsigned> > IndirectSymbolInits;
   std::vector<std::pair<Function*, unsigned> > FunctionPrefixes;
   std::vector<std::pair<Function*, unsigned> > FunctionPrologues;
   std::vector<std::pair<Function*, unsigned> > FunctionPersonalityFns;
 
-  SmallVector<Instruction*, 64> InstsWithTBAATag;
-
   bool HasSeenOldLoopTags = false;
 
   /// The set of attributes by index.  Index zero in the file is for null, and
   /// is thus not represented here.  As such all indices are off by one.
   std::vector<AttributeSet> MAttributes;
 
   /// The set of attribute groups.
   std::map<unsigned, AttributeSet> MAttributeGroups;
case bitc::METADATA_ATTACHMENT: {
           break;
         MDNode *MD = dyn_cast_or_null<MDNode>(Node);
         if (!MD)
           return error("Invalid metadata attachment");
 
         if (HasSeenOldLoopTags && I->second == LLVMContext::MD_loop)
           MD = upgradeInstructionLoopAttachment(*MD);
 
-        Inst->setMetadata(I->second, MD);
         if (I->second == LLVMContext::MD_tbaa) {
-          InstsWithTBAATag.push_back(Inst);
-          continue;
+          assert(!MD->isTemporary() && "should load MDs before attachments");
+          MD = UpgradeTBAANode(*MD);
         }
+        Inst->setMetadata(I->second, MD);
       }
       break;
     }
     }
   }
 }
 
 static std::error_code typeCheckLoadStoreInst(Type *ValType, Type *PtrType) {
if (LastFunctionBlockBit || NextUnreadBit)
     parseModule(LastFunctionBlockBit > NextUnreadBit ? LastFunctionBlockBit
                                                      : NextUnreadBit);
 
   // Check that all block address forward references got resolved (as we
   // promised above).
   if (!BasicBlockFwdRefs.empty())
     return error("Never resolved function from blockaddress");
 
-  // Upgrading intrinsic calls before TBAA can cause TBAA metadata to be lost,
-  // to prevent this instructions with TBAA tags should be upgraded first.
-  for (unsigned I = 0, E = InstsWithTBAATag.size(); I < E; I++)
-    UpgradeInstWithTBAATag(InstsWithTBAATag[I]);
-
   // Upgrade any intrinsic calls that slipped through (should not happen!) and
   // delete the old functions to clean up. We can't do this unless the entire
   // module is materialized because there could always be another function body
   // with calls to the old function.
   for (auto &I : UpgradedIntrinsics) {
     for (auto *U : I.first->users()) {
       if (CallInst *CI = dyn_cast<CallInst>(U))
         UpgradeIntrinsicCall(CI, I.second);

llvm/trunk/lib/IR/AutoUpgrade.cpp

for (auto UI = F->user_begin(), UE = F->user_end(); UI != UE; )
       if (CallInst *CI = dyn_cast<CallInst>(*UI++))
         UpgradeIntrinsicCall(CI, NewFn);
 
     // Remove old function, no longer used, from the module.
     F->eraseFromParent();
   }
 }
 
-void llvm::UpgradeInstWithTBAATag(Instruction *I) {
-  MDNode *MD = I->getMetadata(LLVMContext::MD_tbaa);
-  assert(MD && "UpgradeInstWithTBAATag should have a TBAA tag");
+MDNode *llvm::UpgradeTBAANode(MDNode &MD) {
   // Check if the tag uses struct-path aware TBAA format.
-  if (isa<MDNode>(MD->getOperand(0)) && MD->getNumOperands() >= 3)
-    return;
+  if (isa<MDNode>(MD.getOperand(0)) && MD.getNumOperands() >= 3)
+    return &MD;
 
-  if (MD->getNumOperands() == 3) {
-    Metadata *Elts[] = {MD->getOperand(0), MD->getOperand(1)};
-    MDNode *ScalarType = MDNode::get(I->getContext(), Elts);
+  auto &Context = MD.getContext();
+  if (MD.getNumOperands() == 3) {
+    Metadata *Elts[] = {MD.getOperand(0), MD.getOperand(1)};
+    MDNode *ScalarType = MDNode::get(Context, Elts);
     // Create a MDNode <ScalarType, ScalarType, offset 0, const>
     Metadata *Elts2[] = {ScalarType, ScalarType,
-                         ConstantAsMetadata::get(Constant::getNullValue(
-                             Type::getInt64Ty(I->getContext()))),
-                         MD->getOperand(2)};
-    I->setMetadata(LLVMContext::MD_tbaa, MDNode::get(I->getContext(), Elts2));
-  } else {
-    // Create a MDNode <MD, MD, offset 0>
-    Metadata *Elts[] = {MD, MD, ConstantAsMetadata::get(Constant::getNullValue(
-                                    Type::getInt64Ty(I->getContext())))};
-    I->setMetadata(LLVMContext::MD_tbaa, MDNode::get(I->getContext(), Elts));
+                         ConstantAsMetadata::get(
+                             Constant::getNullValue(Type::getInt64Ty(Context))),
+                         MD.getOperand(2)};
+    return MDNode::get(Context, Elts2);
   }
+  // Create a MDNode <MD, MD, offset 0>
+  Metadata *Elts[] = {&MD, &MD, ConstantAsMetadata::get(Constant::getNullValue(
+                                    Type::getInt64Ty(Context)))};
+  return MDNode::get(Context, Elts);
 }
 
 Instruction *llvm::UpgradeBitCastInst(unsigned Opc, Value *V, Type *DestTy,
                                       Instruction *&Temp) {
   if (Opc != Instruction::BitCast)
     return nullptr;
 
   Temp = nullptr;

llvm/trunk/test/LTO/X86/Inputs/remangle_intrinsics_tbaa.ll

+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+%some_named_struct = type { i8, i8 }
+
+define void @bar(%some_named_struct*) {
+  ret void
+}

llvm/trunk/test/LTO/X86/remangle_intrinsics_tbaa.ll

+; RUN: llvm-as %s -o %t1.bc
+; RUN: llvm-as %p/Inputs/remangle_intrinsics_tbaa.ll -o %t2.bc
+; RUN: llvm-link -disable-lazy-loading %t2.bc %t1.bc -S | FileCheck %s
+
+; Verify that we correctly rename the intrinsic and don't crash
+; CHECK: @llvm.masked.store.v4p0some_named_struct.0.p0v4p0some_named_struct.0
+
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+%some_named_struct = type { i8 }
+
+define void @foo(%some_named_struct*) {
+  call void @llvm.masked.store.v4p0some_named_struct.p0v4p0some_named_struct(<4 x %some_named_struct*> undef, <4 x %some_named_struct*>* undef, i32 8, <4 x i1> undef), !tbaa !5
+  ret void
+}
+
+declare void @llvm.masked.store.v4p0some_named_struct.p0v4p0some_named_struct(<4 x %some_named_struct*>, <4 x %some_named_struct*>*, i32, <4 x i1>) #1
+
+!5 = !{!6, !6, i64 0}
+!6 = !{!"any pointer", !7, i64 0}
+!7 = !{!"omnipotent char", !8, i64 0}
+!8 = !{!"Simple C/C++ TBAA"}

llvm/trunk/tools/llvm-link/llvm-link.cpp

 
 static cl::opt<bool>
 OnlyNeeded("only-needed", cl::desc("Link only needed symbols"));
 
 static cl::opt<bool>
 Force("f", cl::desc("Enable binary output on terminals"));
 
 static cl::opt<bool>
-OutputAssembly("S",
-         cl::desc("Write output as LLVM assembly"), cl::Hidden);
+    DisableLazyLoad("disable-lazy-loading",
+                    cl::desc("Enable binary output on terminals"));
+
+static cl::opt<bool>
+    OutputAssembly("S", cl::desc("Write output as LLVM assembly"), cl::Hidden);
 
 static cl::opt<bool>
 Verbose("v", cl::desc("Print information about actions taken"));
 
 static cl::opt<bool>
 DumpAsm("d", cl::desc("Print assembly as linked"), cl::Hidden);
 
 static cl::opt<bool>

 // link path for the specified file to try to find it...
 //
 static std::unique_ptr<Module> loadFile(const char *argv0,
                                         const std::string &FN,
                                         LLVMContext &Context,
                                         bool MaterializeMetadata = true) {
   SMDiagnostic Err;
   if (Verbose) errs() << "Loading '" << FN << "'\n";
-  std::unique_ptr<Module> Result =
-      getLazyIRFileModule(FN, Err, Context, !MaterializeMetadata);
+  std::unique_ptr<Module> Result;
+  if (DisableLazyLoad)
+    Result = parseIRFile(FN, Err, Context);
+  else
+    Result = getLazyIRFileModule(FN, Err, Context, !MaterializeMetadata);
+
   if (!Result) {
     Err.print(argv0, errs());
     return nullptr;
   }
 
   if (MaterializeMetadata) {
     Result->materializeMetadata();
     UpgradeDebugInfo(*Result);