[PATCH] AsmParser: Add a function to parse a standalone type and value
Alex L
arphaman at gmail.com
Fri Jun 5 14:40:01 PDT 2015
2015-06-05 13:06 GMT-07:00 Duncan P. N. Exon Smith <dexonsmith at apple.com>:
>
> > 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?
>
The updated patch has the updated header. Should I pre-commit the header
update
separately?
>
> > +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()`.
>
I've updated the patch to parse all constant values. I think that the
function
should parse any constant, even constant expressions. Then the user can
figure out
if the constant is valid for their use case or not.
>From what I have seen so far, MachineConstantPool doesn't store constant
arrays
or structs, but I think it can store constant vectors, so the scalar only
version of this
function won't really work.
> > + 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
> >
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150605/f0295155/attachment.html>
More information about the llvm-commits
mailing list