[PATCH] D17664: Move discriminator assignment to the right place.

[David Blaikie via llvm-commits]

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?

On Fri, Feb 26, 2016 at 4:23 PM, Dehao Chen via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> danielcdh created this revision.
> danielcdh added reviewers: dnovillo, davidxl.
> danielcdh added a subscriber: llvm-commits.
>
> Now discriminator is assigned per-function instead of per-module.
>
> http://reviews.llvm.org/D17664
>
> Files:
>   include/llvm/IR/DebugInfoMetadata.h
>   lib/IR/DebugInfoMetadata.cpp
>   lib/Transforms/Utils/AddDiscriminators.cpp
>
> Index: lib/Transforms/Utils/AddDiscriminators.cpp
> ===================================================================
> --- lib/Transforms/Utils/AddDiscriminators.cpp
> +++ lib/Transforms/Utils/AddDiscriminators.cpp
> @@ -173,8 +173,10 @@
>    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
> @@ -199,8 +201,7 @@
>          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()));
> @@ -230,8 +231,10 @@
>            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()));
> Index: lib/IR/DebugInfoMetadata.cpp
> ===================================================================
> --- lib/IR/DebugInfoMetadata.cpp
> +++ lib/IR/DebugInfoMetadata.cpp
> @@ -66,23 +66,6 @@
>                     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)
> Index: include/llvm/IR/DebugInfoMetadata.h
> ===================================================================
> --- include/llvm/IR/DebugInfoMetadata.h
> +++ include/llvm/IR/DebugInfoMetadata.h
> @@ -1192,15 +1192,6 @@
>    /// 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)
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160226/e113ce58/attachment.html>