[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-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));

> +  Mapping->NumberedGlobalValues = NumberedVals;
> +}
> +
>  //===----------------------------------------------------------------------===//
>  // Top-Level Entities
>  //===----------------------------------------------------------------------===//
> Index: unittests/AsmParser/AsmParserTest.cpp
> ===================================================================
> --- unittests/AsmParser/AsmParserTest.cpp
> +++ unittests/AsmParser/AsmParserTest.cpp
> @@ -44,4 +45,23 @@
>  #endif
>  #endif
>  
> +TEST(AsmParserTest, SlotMappingTest) {
> +  LLVMContext &Ctx = getGlobalContext();
> +  StringRef Source = "@0 = global i32 0\n !0 = !{}\n !42 = !{i32 42}";
> +  SMDiagnostic Error;
> +  SlotMapping Mapping;
> +  auto Mod = parseAssemblyString(Source, Error, Ctx, &Mapping);
> +
> +  EXPECT_TRUE(Mod != nullptr);
> +  EXPECT_TRUE(Error.getMessage().empty());
> +
> +  ASSERT_EQ(Mapping.NumberedGlobalValues.size(), size_t(1));
> +  EXPECT_TRUE(isa<GlobalVariable>(Mapping.NumberedGlobalValues[0]));
> +
> +  EXPECT_EQ(Mapping.NumberedMetadata.size(), size_t(2));

You just need something unsigned here to avoid warnings, right?  This is
a better way:

    EXPECT_EQ(Mapping.NumberedMetadata.size(), 2u);

> +  EXPECT_EQ(Mapping.NumberedMetadata.count(0), size_t(1));
> +  EXPECT_EQ(Mapping.NumberedMetadata.count(42), size_t(1));
> +  EXPECT_EQ(Mapping.NumberedMetadata.count(1), size_t(0));
> +}
> +
>  } // end anonymous namespace
>