[PATCH] AsmParser: Extend the API to make the mappings from the numbers in the source to unnamed global values accessible to the library users.

[Duncan P. N. Exon Smith]

> On 2015-Jun-22, at 17:09, Alex L <arphaman at gmail.com> wrote:
> 
> 
> 
> 2015-06-18 17:21 GMT-07:00 Duncan P. N. Exon Smith <dexonsmith at apple.com>:
> 
> > On 2015-Jun-18, at 14:59, Alex Lorenz <arphaman at gmail.com> wrote:
> >
> > Hi dexonsmith, bob.wilson, bogner,
> >
> > This patch creates a new SlotMapping struct in the AsmParser library which can be passed into the publicly accessible parsing functions in order to extract the data structures that map from numbers to global values and metadata nodes.
> >
> > This patch would be useful for MIR serialization, as it would allow the MIR parser to lookup global values by their number and metadata nodes by their number.
> >
> > REPOSITORY
> >  rL LLVM
> >
> > http://reviews.llvm.org/D10551
> >
> > Files:
> >  include/llvm/AsmParser/Parser.h
> >  include/llvm/AsmParser/SlotMapping.h
> >  lib/AsmParser/LLParser.cpp
> >  lib/AsmParser/LLParser.h
> >  lib/AsmParser/Parser.cpp
> >  unittests/AsmParser/AsmParserTest.cpp
> >
> > EMAIL PREFERENCES
> >  http://reviews.llvm.org/settings/panel/emailpreferences/
> > <D10551.27965.patch>
> 
> This LGTM with some fixups.
> 
> > Index: include/llvm/AsmParser/SlotMapping.h
> > ===================================================================
> > --- /dev/null
> > +++ include/llvm/AsmParser/SlotMapping.h
> > @@ -0,0 +1,34 @@
> > +//===-- SlotMapping.h - Slot number mapping for unnamed values --*- C++ -*-===//
> > +//
> > +//                     The LLVM Compiler Infrastructure
> > +//
> > +// This file is distributed under the University of Illinois Open Source
> > +// License. See LICENSE.TXT for details.
> > +//
> > +//===----------------------------------------------------------------------===//
> > +//
> > +// This file contains the declaration of the SlotMapping class.
> > +//
> > +//===----------------------------------------------------------------------===//
> > +
> > +#ifndef LLVM_ASMPARSER_SLOTMAPPING_H
> > +#define LLVM_ASMPARSER_SLOTMAPPING_H
> > +
> > +#include <map>
> > +#include <vector>
> > +
> > +namespace llvm {
> > +
> > +class GlobalValue;
> > +class MDNode;
> > +
> > +/// This class contains the mapping from the slot numbers to unnamed metadata
> > +/// nodes and global values.
> > +struct SlotMapping {
> > +  std::vector<GlobalValue *> NumberedGlobalValues;
> > +  std::map<unsigned, const MDNode *> NumberedMetadata;
> 
> Remove the `Numbered` from the field names.  It's implied by the
> context.
> 
> I might even use the more succinct `GVs` and `MDs`, but it's up to you.
> 
> > +};
> > +
> > +} // end namespace llvm
> > +
> > +#endif
> > Index: lib/AsmParser/LLParser.cpp
> > ===================================================================
> > --- lib/AsmParser/LLParser.cpp
> > +++ lib/AsmParser/LLParser.cpp
> > @@ -161,9 +162,19 @@
> >
> >    UpgradeDebugInfo(*M);
> >
> > +  constructSlotMapping();
> > +
> >    return false;
> >  }
> >
> > +void LLParser::constructSlotMapping() {
> > +  if (!Mapping)
> > +    return;
> > +  for (const auto &N : NumberedMetadata)
> > +    Mapping->NumberedMetadata.insert(std::make_pair(N.first, N.second.get()));
> 
> This is more efficient, O(n) instead of O(n lg n):
> 
>     std::copy(NumberedMetadata.begin(), NumberedMetadata.end(),
>               std::back_inserter(Mapping->NumberedMetadata));
> 
> This didn't work for me,

Ah, should have been:

    std::inserter(Mapping->NumberedMetadata,
                  Mapping->NumberedMetadata.end());

> but for efficiency I decided to just std::move the data structures
> as LLParser doesn't need them anymore anyway.
> Please check out the updated patch :)

LGTM.  Although:

>  /// This function is the main interface to the LLVM Assembly Parser. It parses
>  /// an ASCII file that (presumably) contains LLVM Assembly code. It returns a
>  /// Module (intermediate representation) with the corresponding features. Note
>  /// that this does not verify that the generated Module is valid, so you should
>  /// run the verifier after parsing the file to check that it is okay.
> -/// @brief Parse LLVM Assembly from a file
> -/// @param Filename The name of the file to parse
> -/// @param Error Error result info.
> -/// @param Context Context in which to allocate globals info.
> +/// \brief Parse LLVM Assembly from a file
> +/// \param Filename The name of the file to parse
> +/// \param Error Error result info.
> +/// \param Context Context in which to allocate globals info.
> +/// \param Mapping The optional slot mapping that will be initialized during
> +///                parsing.
>  std::unique_ptr<Module> parseAssemblyFile(StringRef Filename,
>                                            SMDiagnostic &Error,
> -                                          LLVMContext &Context);
> +                                          LLVMContext &Context,
> +                                          SlotMapping *Mapping = nullptr);
> 

I kind of prefer `Slots` as a name rather than `Mapping` (also in
the rest of the patch).  Feel free to disagree.