[PATCH] D32975: Make it illegal for two Functions to point to the same DISubprogram

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: unittests/Transforms/Utils/Cloning.cpp:391-392
       // But that they belong to different functions
-      auto *OldSubprogram = cast<DISubprogram>(OldDL.getScope());
-      auto *NewSubprogram = cast<DISubprogram>(NewDL.getScope());
+      auto *OldSubprogram = cast<DISubprogram>(OldDL.getInlinedAtScope());
+      auto *NewSubprogram = cast<DISubprogram>(NewDL.getInlinedAtScope());
       EXPECT_EQ(OldFunc->getSubprogram(), OldSubprogram);
----------------
aprantl wrote:
> dblaikie wrote:
> > I don't really understand why this change is here - could you explain it?
> > 
> > I guess the actual change you made to CloneFunction makes the new function look as though the old function was inlined into it? That's a great idea/way to do it! :) (though won't be what I need for the LTO+Fission issue - but perhaps I can use some of the pieces you've built/generalized/etc here)
> Much simpler: This change is necessary because above, I'm adding an inlined function into OldFunc (line 309ff). DILocation::getInlinedAtScope() returns either the parent function of a regular instruction or the parent of the inlined-at location of an inlined instruction, both of which should be the DISubprogram attached to the function the instruction is part of.
Hmm, OK - could you explain more why you're doing that? To test the inlining scope rewriting functionality? That's a good point/something I'll have to deal with as well for the ThinLTO case - the frontend compilation might've already done some inlining... oh, except those won't ever be from another module, so that's easier... unless the module linking messed that up, maybe, and picked the foreign module's version of the subprogram :/ Will have to check/ponder.

But this also raises an interesting question then: what if we did represent function cloning as an act of inlining? That might actually be better? maybe? (I wouldn't do it in this commit, probably better separately)


https://reviews.llvm.org/D32975