[PATCH] AsmParser: Add a function to parse a standalone type and value

[Duncan P. N. Exon Smith]

> On 2015-Jun-05, at 11:29, Alex Lorenz <arphaman at gmail.com> wrote:
> 
> Hi dexonsmith, bogner, bob.wilson,
> 
> This patch extends the interface provided by the AsmParser library by adding a function that allows the user to parse a standalone type and value in a given string.
> 
> This change would be useful for the MachineIR serialization, as it would allow the MIR parser to parse the constant values in a MachineConstantPool. 
> This is an example of a MIR fragment that shows several constant pool entries in a machine function:
> 
>  constants:
>    - value:           'double 3.250000e+00'
>    - value:           'float 6.250000e+00'
>      alignment:       4
> 
> REPOSITORY
>  rL LLVM
> 
> http://reviews.llvm.org/D10280
> 
> Files:
>  include/llvm/AsmParser/Parser.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/

> Index: include/llvm/AsmParser/Parser.h
> ===================================================================
> --- include/llvm/AsmParser/Parser.h
> +++ include/llvm/AsmParser/Parser.h
> @@ -21,6 +21,7 @@
>  class Module;
>  class SMDiagnostic;
>  class LLVMContext;
> +class Value;
>  
>  /// 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
> @@ -67,6 +68,12 @@
>  /// @return true on error.
>  bool parseAssemblyInto(MemoryBufferRef F, Module &M, SMDiagnostic &Err);
>  
> +/// Parses a type and a value in the given string.
> +///
> +/// @return null on error.

I think I've seen the LaTeX variant (`\return`) more often than the
JavaDoc variant (`@return`) in LLVM, and I have a mild preference for
it.  Unless you disagree with this colour of paint, can you update the
header to use this style, and then change your patch to use it?

> +const Value *parseTypeAndValue(StringRef AsmString, SMDiagnostic &Err,
> +                               const Module &M);

I can't see how this could be useful outside of constants.  Unless you
see a use, please change this to return `Constant *`.  (No reason for
the `const` in the return, I don't think.)

Might even want to make this *more* specific... see below.

> +
>  } // End llvm namespace
>  
>  #endif
> Index: lib/AsmParser/LLParser.cpp
> ===================================================================
> --- lib/AsmParser/LLParser.cpp
> +++ lib/AsmParser/LLParser.cpp
> @@ -47,6 +47,19 @@
>           ValidateEndOfModule();
>  }
>  
> +const Value *LLParser::parseStandaloneTypeAndValue() {
> +  Lex.Lex();
> +
> +  Value *V;
> +  if (ParseTypeAndValue(V, /*PFS=*/nullptr))
> +    return nullptr;

I'm not sure this is careful enough.  What do you do for this?

    i32* @var

Is that allowed?  If so, I'm confused (and what about functions?).  If
not, you should do some checking.

  - You should check that this is a constant (there may even be a
    `ParseConstant()` function?).
  - Since this is standalone, you should check that it's not in any way
    referencing a `GlobalValue` (or, if they're reasonable here, please
    explain what I'm missing).
  - Should this support aggregate types?  You should be clear in the
    doxygen of the top-level function, and error out appropriately.
  - Should this support `ConstantExpr`?  Again, be clear in the doxygen
    and error out appropriately.

BTW, I think checking `isa<ConstantFP>(C) || isa<ConstantInt>(C)` would
be pretty reasonable, and would rule out all the corner cases I
mentioned.  You might even add a new class `ConstantScalar` that those
two inherit from, and change the top-level function to:

    ConstantScalar *parseConstantScalar(StringRef Asm, ...);

and rename this to `LLParser::parseStandaloneConstantScalar()`.

> +  if (Lex.getKind() != lltok::Eof) {
> +    Error(Lex.getLoc(), "expected end of string");
> +    return nullptr;
> +  }
> +  return V;
> +}
> +
>  /// ValidateEndOfModule - Do final validity and sanity checks at the end of the
>  /// module.
>  bool LLParser::ValidateEndOfModule() {
> Index: lib/AsmParser/LLParser.h
> ===================================================================
> --- lib/AsmParser/LLParser.h
> +++ lib/AsmParser/LLParser.h
> @@ -140,6 +140,8 @@
>            BlockAddressPFS(nullptr) {}
>      bool Run();
>  
> +    const Value *parseStandaloneTypeAndValue();
> +
>      LLVMContext &getContext() { return Context; }
>  
>    private:
> Index: lib/AsmParser/Parser.cpp
> ===================================================================
> --- lib/AsmParser/Parser.cpp
> +++ lib/AsmParser/Parser.cpp
> @@ -62,3 +62,12 @@
>    MemoryBufferRef F(AsmString, "<string>");
>    return parseAssembly(F, Err, Context);
>  }
> +
> +const Value *llvm::parseTypeAndValue(StringRef AsmString, SMDiagnostic &Err,
> +                                     const Module &M) {
> +  SourceMgr SM;
> +  std::unique_ptr<MemoryBuffer> Buf = MemoryBuffer::getMemBuffer(AsmString);
> +  SM.AddNewSourceBuffer(std::move(Buf), SMLoc());
> +  return LLParser(AsmString, SM, Err, const_cast<Module *>(&M))
> +      .parseStandaloneTypeAndValue();
> +}
> Index: unittests/AsmParser/AsmParserTest.cpp
> ===================================================================
> --- unittests/AsmParser/AsmParserTest.cpp
> +++ unittests/AsmParser/AsmParserTest.cpp
> @@ -9,6 +9,7 @@
>  
>  #include "llvm/ADT/StringRef.h"
>  #include "llvm/AsmParser/Parser.h"
> +#include "llvm/IR/Constants.h"
>  #include "llvm/IR/LLVMContext.h"
>  #include "llvm/IR/Module.h"
>  #include "llvm/Support/SourceMgr.h"
> @@ -44,4 +45,33 @@
>  #endif
>  #endif
>  
> +TEST(AsmParserTest, TypeAndValueParsing) {
> +  LLVMContext &Ctx = getGlobalContext();
> +  Module M("test", Ctx);
> +  SMDiagnostic Error;
> +  const Value *V;
> +
> +  V = parseTypeAndValue("double 3.5", Error, M);
> +  EXPECT_TRUE(V != nullptr);

Either use:

    EXPECT_NE(nullptr, V);

Or, my preference:

    EXPECT_TRUE(V);

> +  EXPECT_TRUE(V->getType()->isDoubleTy());
> +  EXPECT_TRUE(isa<ConstantFP>(V));
> +  EXPECT_TRUE(cast<ConstantFP>(V)->isExactlyValue(3.5));

This will assert out and crash if `V` isn't a `ConstantFP`.  Maybe the
previous check for `isa` should be `ASSERT_TRUE` so that the test is
aborted on failure without hiding results from other tests.

> +
> +  V = parseTypeAndValue("i32 42", Error, M);
> +  EXPECT_TRUE(V != nullptr);
> +  EXPECT_TRUE(V->getType()->isIntegerTy());
> +  EXPECT_TRUE(isa<ConstantInt>(V));
> +  EXPECT_TRUE(cast<ConstantInt>(V)->equalsInt(42));
> +
> +  V = parseTypeAndValue("duble 3.25", Error, M);
> +  EXPECT_FALSE(V != nullptr);

This double negative is confusing.  Here you're not even using the
result `V`, so IMO you should just say:

    EXPECT_FALSE(parseTypeAndValue(...));

Also, should you check `Error`?

> +
> +  V = parseTypeAndValue("i32 3.25", Error, M);
> +  EXPECT_FALSE(V != nullptr);

Should you check `Error`?

> +
> +  V = parseTypeAndValue("i32 3, ", Error, M);
> +  EXPECT_FALSE(V != nullptr);
> +  EXPECT_EQ(Error.getMessage(), "expected end of string");
> +}
> +
>  } // end anonymous namespace
>