Cloning: Reduce complexity of debug info cloning and fix correctness issue.

[pcc]

Summary

Commit r260791 contained an error in that it would introduce a cross-module
reference in the old module. It also introduced O(N^2) complexity in the
module cloner by requiring the entire module to be visited for each function.
Fix both of these problems by avoiding use of the CloneDebugInfoMetadata
function (which is only designed to do intra-module cloning) and cloning
function-attached metadata in the same way that we clone all other metadata.

[dexonsmith]

If you add a test this LGTM. I suggest adding something to
unittests/Transforms/Utils/.

[pcc]

This is already covered by the test CloneModule, Subprogram that was added by r260791. But I suppose I could add another test for another attribute since that is new functionality.

[dexonsmith]

That test passes without this patch...

Can you test specifically for the bug you're fixing?

[pcc]

Most likely, let me see if I can reproduce the issue that Teresa found (I was mostly looking at the complexity issue).

[tejohnson]

LGTM, but would be good for Adrian or Duncan to take a look.

lib/Transforms/Utils/CloneFunction.cpp

194 ↗

(On Diff #52001)

Maybe add a comment about this applying only to cloning within a single module?

[pcc]

Sorry, looks like I'll have to go back to the drawing board with this one, it causes test failures in check-clang.

[pcc]

• Pass ModuleLevelChanges through to MapMetadata, and add test

[pcc]

The problems were related to the calls to CloneFunction in lib/CodeGen/CGVTables.cpp` and in lib/Target/AMDGPU/AMDGPUAlwaysInlinePass.cpp. In both cases we were wrongly cloning distinct debug info, which confused later codegen/verifier passes. This update fixes the inconsistency. Please take another look.

@tstellarAMD it's a little surprising that the only test coverage for the always inline pass in the case where there's debug info is in Clang (specifically in test/CodeGen/backend-unsupported-error.ll, which seems to test something else). Might be something worth adding coverage for in LLVM?

[pcc]

lib/Transforms/Utils/CloneFunction.cpp

198–199 ↗

(On Diff #52124)

I have a followup patch which will completely remove this function.

[dexonsmith]

LGTM.

llvm/trunk/include/llvm/Transforms/Utils/Cloning.h

 ///
 /// If ModuleLevelChanges is false, VMap contains no non-identity GlobalValue
 /// mappings, and debug info metadata will not be cloned.
 ///
 Function *CloneFunction(const Function *F, ValueToValueMapTy &VMap,
                         bool ModuleLevelChanges,
                         ClonedCodeInfo *CodeInfo = nullptr);
 
-/// Clone the module-level debug info associated with OldFunc. The cloned data
-/// will point to NewFunc instead.
-void CloneDebugInfoMetadata(Function *NewFunc, const Function *OldFunc,
-                            ValueToValueMapTy &VMap);
-
 /// Clone OldFunc into NewFunc, transforming the old arguments into references
 /// to VMap values.  Note that if NewFunc already has basic blocks, the ones
 /// cloned into it will be added to the end of the function.  This function
 /// fills in a list of return instructions, and can optionally remap types
 /// and/or append the specified suffix to all values cloned.
 ///
 /// If ModuleLevelChanges is false, VMap contains no non-identity GlobalValue
 /// mappings.

llvm/trunk/lib/Transforms/Utils/CloneFunction.cpp

#endif
 
   NewFunc->setAttributes(
       NewFunc->getAttributes()
           .addAttributes(NewFunc->getContext(), AttributeSet::ReturnIndex,
                          OldAttrs.getRetAttributes())
           .addAttributes(NewFunc->getContext(), AttributeSet::FunctionIndex,
                          OldAttrs.getFnAttributes()));
 
+  SmallVector<std::pair<unsigned, MDNode *>, 1> MDs;
+  OldFunc->getAllMetadata(MDs);
+  for (auto MD : MDs)
+    NewFunc->setMetadata(
+        MD.first,
+        MapMetadata(MD.second, VMap,
+                    ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges,
+                    TypeMapper, Materializer));
+
   // Loop over all of the basic blocks in the function, cloning them as
   // appropriate.  Note that we save BE this way in order to handle cloning of
   // recursive functions into themselves.
   //
   for (Function::const_iterator BI = OldFunc->begin(), BE = OldFunc->end();
        BI != BE; ++BI) {
     const BasicBlock &BB = *BI;
 
static void AddOperand(DICompileUnit *CU, DISubprogramArray SPs,
   for (auto *SP : SPs)
     NewSPs.push_back(SP);
   NewSPs.push_back(NewSP);
   CU->replaceSubprograms(MDTuple::get(CU->getContext(), NewSPs));
 }
 
 // Clone the module-level debug info associated with OldFunc. The cloned data
 // will point to NewFunc instead.
-void llvm::CloneDebugInfoMetadata(Function *NewFunc, const Function *OldFunc,
+static void CloneDebugInfoMetadata(Function *NewFunc, const Function *OldFunc,
                                    ValueToValueMapTy &VMap) {
   DebugInfoFinder Finder;
   Finder.processModule(*OldFunc->getParent());
 
   const DISubprogram *OldSubprogramMDNode = FindSubprogram(OldFunc, Finder);
   if (!OldSubprogramMDNode) return;
 
   auto *NewSubprogram =
       cast<DISubprogram>(MapMetadata(OldSubprogramMDNode, VMap));

llvm/trunk/lib/Transforms/Utils/CloneModule.cpp

for (Module::const_iterator I = M->begin(), E = M->end(); I != E; ++I) {
     if (!I->isDeclaration()) {
       Function::arg_iterator DestI = F->arg_begin();
       for (Function::const_arg_iterator J = I->arg_begin(); J != I->arg_end();
            ++J) {
         DestI->setName(J->getName());
         VMap[&*J] = &*DestI++;
       }
 
-      CloneDebugInfoMetadata(F, &*I, VMap);
       SmallVector<ReturnInst*, 8> Returns;  // Ignore returns cloned.
       CloneFunctionInto(F, &*I, VMap, /*ModuleLevelChanges=*/true, Returns);
     }
 
     if (I->hasPersonalityFn())
       F->setPersonalityFn(MapValue(I->getPersonalityFn(), VMap));
   }
 

llvm/trunk/unittests/Transforms/Utils/Cloning.cpp

protected:
   Module *OldM;
   Module *NewM;
 };
 
 TEST_F(CloneModule, Verify) {
   EXPECT_FALSE(verifyModule(*NewM));
 }
 
+TEST_F(CloneModule, OldModuleUnchanged) {
+  DebugInfoFinder Finder;
+  Finder.processModule(*OldM);
+  EXPECT_EQ(1U, Finder.subprogram_count());
+}
+
 TEST_F(CloneModule, Subprogram) {
   Function *NewF = NewM->getFunction("f");
   DISubprogram *SP = NewF->getSubprogram();
   EXPECT_TRUE(SP != nullptr);
   EXPECT_EQ(SP->getName(), "f");
   EXPECT_EQ(SP->getFile()->getFilename(), "filename.c");
   EXPECT_EQ(SP->getLine(), (unsigned)4);
 }
 }