Move discriminator assignment to the right place.

[danielcdh]

Summary

Now discriminator is assigned per-function instead of per-module.

[dblaikie]

I'm sure it's probably obvious, but you've not mentioned it here - why is
the "right place" the function, and not the module?

[danielcdh]

It was mentioned in the FIXME comment I just removed:

• // FIXME: This seems completely wrong.
• //
• // 1. If two modules are generated in the same context, then the second
• // Module will get different discriminators than it would have if it were
• // generated in its own context.
• // 2. If this function is called after round-tripping to bitcode instead of
• // before, it will give a different (and potentially incorrect!) return.
• //
• // The discriminator should instead be calculated from local information
• // where it's actually needed. This logic should be moved to
• // AddDiscriminators::runOnFunction(), where it doesn't pollute the
• // LLVMContext.

[dblaikie]

Ah, great - thanks! (& I'd read that, but was reading the patch backwards,
as though that comment was being added, looking at it in more detail the
patch does look great/going in the right direction to be sure - will leave
it to Diego or others to sign off on, though)

[dnovillo]

LGTM. Thanks for the fix!

include/llvm/IR/DebugInfoMetadata.h

public:
   }
 
   /// \brief Get the DWARF discriminator.
   ///
   /// DWARF discriminators distinguish identical file locations between
   /// instructions that are on different basic blocks.
   inline unsigned getDiscriminator() const;
 
-  /// \brief Compute new discriminator in the given context.
-  ///
-  /// This modifies the \a LLVMContext that \c this is in to increment the next
-  /// discriminator for \c this's line/filename combination.
-  ///
-  /// FIXME: Delete this.  See comments in implementation and at the only call
-  /// site in \a AddDiscriminators::runOnFunction().
-  unsigned computeNewDiscriminator() const;
-
   Metadata *getRawScope() const { return getOperand(0); }
   Metadata *getRawInlinedAt() const {
     if (getNumOperands() == 2)
       return getOperand(1);
     return nullptr;
   }
 
   static bool classof(const Metadata *MD) {

lib/IR/DebugInfoMetadata.cpp

DILocation *DILocation::getImpl(LLVMContext &Context, unsigned Line,
   Ops.push_back(Scope);
   if (InlinedAt)
     Ops.push_back(InlinedAt);
   return storeImpl(new (Ops.size())
                        DILocation(Context, Storage, Line, Column, Ops),
                    Storage, Context.pImpl->DILocations);
 }
 
-unsigned DILocation::computeNewDiscriminator() const {
-  // FIXME: This seems completely wrong.
-  //
-  //  1. If two modules are generated in the same context, then the second
-  //     Module will get different discriminators than it would have if it were
-  //     generated in its own context.
-  //  2. If this function is called after round-tripping to bitcode instead of
-  //     before, it will give a different (and potentially incorrect!) return.
-  //
-  // The discriminator should instead be calculated from local information
-  // where it's actually needed.  This logic should be moved to
-  // AddDiscriminators::runOnFunction(), where it doesn't pollute the
-  // LLVMContext.
-  std::pair<const char *, unsigned> Key(getFilename().data(), getLine());
-  return ++getContext().pImpl->DiscriminatorTable[Key];
-}
-
 unsigned DINode::getFlag(StringRef Flag) {
   return StringSwitch<unsigned>(Flag)
 #define HANDLE_DI_FLAG(ID, NAME) .Case("DIFlag" #NAME, Flag##NAME)
 #include "llvm/IR/DebugInfoFlags.def"
       .Default(0);
 }
 
 const char *DINode::getFlagString(unsigned Flag) {

lib/Transforms/Utils/AddDiscriminators.cpp

bool AddDiscriminators::runOnFunction(Function &F) {
   bool Changed = false;
   Module *M = F.getParent();
   LLVMContext &Ctx = M->getContext();
   DIBuilder Builder(*M, /*AllowUnresolved*/ false);
 
   typedef std::pair<StringRef, unsigned> Location;
   typedef DenseMap<const BasicBlock *, Metadata *> BBScopeMap;
   typedef DenseMap<Location, BBScopeMap> LocationBBMap;
+  typedef DenseMap<Location, unsigned> LocationDiscriminatorMap;
 
   LocationBBMap LBM;
+  LocationDiscriminatorMap LDM;
 
   // Traverse all instructions in the function. If the source line location
   // of the instruction appears in other basic block, assign a new
   // discriminator for this instruction.
   for (BasicBlock &B : F) {
     for (auto &I : B.getInstList()) {
       if (isa<DbgInfoIntrinsic>(&I))
         continue;
       const DILocation *DIL = I.getDebugLoc();
       if (!DIL)
         continue;
       Location L = std::make_pair(DIL->getFilename(), DIL->getLine());
       auto &BBMap = LBM[L];
       auto R = BBMap.insert(std::make_pair(&B, (Metadata *)nullptr));
       if (BBMap.size() == 1)
         continue;
       bool InsertSuccess = R.second;
       Metadata *&NewScope = R.first->second;
       // If we could insert a different block in the same location, a
       // discriminator is needed to distinguish both instructions.
       if (InsertSuccess) {
         auto *Scope = DIL->getScope();
         auto *File =
             Builder.createFile(DIL->getFilename(), Scope->getDirectory());
-        NewScope = Builder.createLexicalBlockFile(
-            Scope, File, DIL->computeNewDiscriminator());
+        NewScope = Builder.createLexicalBlockFile(Scope, File, ++LDM[L]);
       }
       I.setDebugLoc(DILocation::get(Ctx, DIL->getLine(), DIL->getColumn(),
                                     NewScope, DIL->getInlinedAt()));
       DEBUG(dbgs() << DIL->getFilename() << ":" << DIL->getLine() << ":"
                    << DIL->getColumn() << ":"
                    << dyn_cast<DILexicalBlockFile>(NewScope)->getDiscriminator()
                    << I << "\n");
       Changed = true;
for (auto &I : B.getInstList()) {
 
       DILocation *CurrentDIL = Current->getDebugLoc();
       if (FirstDIL) {
         if (CurrentDIL && CurrentDIL->getLine() == FirstDIL->getLine() &&
             CurrentDIL->getFilename() == FirstDIL->getFilename()) {
           auto *Scope = FirstDIL->getScope();
           auto *File = Builder.createFile(FirstDIL->getFilename(),
                                           Scope->getDirectory());
-          auto *NewScope = Builder.createLexicalBlockFile(
-              Scope, File, FirstDIL->computeNewDiscriminator());
+          Location L =
+              std::make_pair(FirstDIL->getFilename(), FirstDIL->getLine());
+          auto *NewScope =
+              Builder.createLexicalBlockFile(Scope, File, ++LDM[L]);
           Current->setDebugLoc(DILocation::get(
               Ctx, CurrentDIL->getLine(), CurrentDIL->getColumn(), NewScope,
               CurrentDIL->getInlinedAt()));
           Changed = true;
         } else {
           FirstDIL = CurrentDIL;
         }
       } else {
         FirstDIL = CurrentDIL;
       }
     }
   }
   return Changed;
 }