<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2015-06-05 13:06 GMT-07:00 Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5"><br>
> On 2015-Jun-05, at 11:29, Alex Lorenz <<a href="mailto:arphaman@gmail.com">arphaman@gmail.com</a>> wrote:<br>
><br>
> Hi dexonsmith, bogner, bob.wilson,<br>
><br>
> 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.<br>
><br>
> This change would be useful for the MachineIR serialization, as it would allow the MIR parser to parse the constant values in a MachineConstantPool.<br>
> This is an example of a MIR fragment that shows several constant pool entries in a machine function:<br>
><br>
> constants:<br>
> - value: 'double 3.250000e+00'<br>
> - value: 'float 6.250000e+00'<br>
> alignment: 4<br>
><br>
> REPOSITORY<br>
> rL LLVM<br>
><br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10280&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=8A9s_rRep528WcVcpTLmKv79u7PoEA_w1q2kRmXiLCQ&s=y3wyOhvx-PCUkhfqNI17HWpKnyUi_i47QcDEJg3Sgeo&e=" target="_blank">http://reviews.llvm.org/D10280</a><br>
><br>
> Files:<br>
> include/llvm/AsmParser/Parser.h<br>
> lib/AsmParser/LLParser.cpp<br>
> lib/AsmParser/LLParser.h<br>
> lib/AsmParser/Parser.cpp<br>
> unittests/AsmParser/AsmParserTest.cpp<br>
><br>
> EMAIL PREFERENCES<br>
> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=8A9s_rRep528WcVcpTLmKv79u7PoEA_w1q2kRmXiLCQ&s=Pl1eked_1DAHPMVJg5fzPy9MGRIt6q8LCRP3w4-sqTo&e=" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
</div></div>> Index: include/llvm/AsmParser/Parser.h<br>
> ===================================================================<br>
> --- include/llvm/AsmParser/Parser.h<br>
> +++ include/llvm/AsmParser/Parser.h<br>
> @@ -21,6 +21,7 @@<br>
> class Module;<br>
> class SMDiagnostic;<br>
> class LLVMContext;<br>
> +class Value;<br>
><br>
> /// This function is the main interface to the LLVM Assembly Parser. It parses<br>
> /// an ASCII file that (presumably) contains LLVM Assembly code. It returns a<br>
> @@ -67,6 +68,12 @@<br>
> /// @return true on error.<br>
> bool parseAssemblyInto(MemoryBufferRef F, Module &M, SMDiagnostic &Err);<br>
><br>
> +/// Parses a type and a value in the given string.<br>
> +///<br>
> +/// @return null on error.<br>
<br>
I think I've seen the LaTeX variant (`\return`) more often than the<br>
JavaDoc variant (`@return`) in LLVM, and I have a mild preference for<br>
it. Unless you disagree with this colour of paint, can you update the<br>
header to use this style, and then change your patch to use it?<br></blockquote><div><br></div><div>The updated patch has the updated header. Should I pre-commit the header update</div><div>separately?</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +const Value *parseTypeAndValue(StringRef AsmString, SMDiagnostic &Err,<br>
> + const Module &M);<br>
<br>
I can't see how this could be useful outside of constants. Unless you<br>
see a use, please change this to return `Constant *`. (No reason for<br>
the `const` in the return, I don't think.)<br>
<br>
Might even want to make this *more* specific... see below.<br>
<br>
> +<br>
> } // End llvm namespace<br>
><br>
> #endif<br>
> Index: lib/AsmParser/LLParser.cpp<br>
> ===================================================================<br>
> --- lib/AsmParser/LLParser.cpp<br>
> +++ lib/AsmParser/LLParser.cpp<br>
> @@ -47,6 +47,19 @@<br>
> ValidateEndOfModule();<br>
> }<br>
><br>
> +const Value *LLParser::parseStandaloneTypeAndValue() {<br>
> + Lex.Lex();<br>
> +<br>
> + Value *V;<br>
> + if (ParseTypeAndValue(V, /*PFS=*/nullptr))<br>
> + return nullptr;<br>
<br>
I'm not sure this is careful enough. What do you do for this?<br>
<br>
i32* @var<br>
<br>
Is that allowed? If so, I'm confused (and what about functions?). If<br>
not, you should do some checking.<br>
<br>
- You should check that this is a constant (there may even be a<br>
`ParseConstant()` function?).<br>
- Since this is standalone, you should check that it's not in any way<br>
referencing a `GlobalValue` (or, if they're reasonable here, please<br>
explain what I'm missing).<br>
- Should this support aggregate types? You should be clear in the<br>
doxygen of the top-level function, and error out appropriately.<br>
- Should this support `ConstantExpr`? Again, be clear in the doxygen<br>
and error out appropriately.<br>
<br>
BTW, I think checking `isa<ConstantFP>(C) || isa<ConstantInt>(C)` would<br>
be pretty reasonable, and would rule out all the corner cases I<br>
mentioned. You might even add a new class `ConstantScalar` that those<br>
two inherit from, and change the top-level function to:<br>
<br>
ConstantScalar *parseConstantScalar(StringRef Asm, ...);<br>
<br>
and rename this to `LLParser::parseStandaloneConstantScalar()`.<br></blockquote><div><br></div><div>I've updated the patch to parse all constant values. I think that the function</div><div>should parse any constant, even constant expressions. Then the user can figure out </div><div>if the constant is valid for their use case or not.</div><div><br></div><div>From what I have seen so far, MachineConstantPool doesn't store constant arrays</div><div>or structs, but I think it can store constant vectors, so the scalar only version of this</div><div>function won't really work.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + if (Lex.getKind() != lltok::Eof) {<br>
> + Error(Lex.getLoc(), "expected end of string");<br>
> + return nullptr;<br>
> + }<br>
> + return V;<br>
> +}<br>
> +<br>
> /// ValidateEndOfModule - Do final validity and sanity checks at the end of the<br>
> /// module.<br>
> bool LLParser::ValidateEndOfModule() {<br>
> Index: lib/AsmParser/LLParser.h<br>
> ===================================================================<br>
> --- lib/AsmParser/LLParser.h<br>
> +++ lib/AsmParser/LLParser.h<br>
> @@ -140,6 +140,8 @@<br>
> BlockAddressPFS(nullptr) {}<br>
> bool Run();<br>
><br>
> + const Value *parseStandaloneTypeAndValue();<br>
> +<br>
> LLVMContext &getContext() { return Context; }<br>
><br>
> private:<br>
> Index: lib/AsmParser/Parser.cpp<br>
> ===================================================================<br>
> --- lib/AsmParser/Parser.cpp<br>
> +++ lib/AsmParser/Parser.cpp<br>
> @@ -62,3 +62,12 @@<br>
> MemoryBufferRef F(AsmString, "<string>");<br>
> return parseAssembly(F, Err, Context);<br>
> }<br>
> +<br>
> +const Value *llvm::parseTypeAndValue(StringRef AsmString, SMDiagnostic &Err,<br>
> + const Module &M) {<br>
> + SourceMgr SM;<br>
> + std::unique_ptr<MemoryBuffer> Buf = MemoryBuffer::getMemBuffer(AsmString);<br>
> + SM.AddNewSourceBuffer(std::move(Buf), SMLoc());<br>
> + return LLParser(AsmString, SM, Err, const_cast<Module *>(&M))<br>
> + .parseStandaloneTypeAndValue();<br>
> +}<br>
> Index: unittests/AsmParser/AsmParserTest.cpp<br>
> ===================================================================<br>
> --- unittests/AsmParser/AsmParserTest.cpp<br>
> +++ unittests/AsmParser/AsmParserTest.cpp<br>
> @@ -9,6 +9,7 @@<br>
><br>
> #include "llvm/ADT/StringRef.h"<br>
> #include "llvm/AsmParser/Parser.h"<br>
> +#include "llvm/IR/Constants.h"<br>
> #include "llvm/IR/LLVMContext.h"<br>
> #include "llvm/IR/Module.h"<br>
> #include "llvm/Support/SourceMgr.h"<br>
> @@ -44,4 +45,33 @@<br>
> #endif<br>
> #endif<br>
><br>
> +TEST(AsmParserTest, TypeAndValueParsing) {<br>
> + LLVMContext &Ctx = getGlobalContext();<br>
> + Module M("test", Ctx);<br>
> + SMDiagnostic Error;<br>
> + const Value *V;<br>
> +<br>
> + V = parseTypeAndValue("double 3.5", Error, M);<br>
> + EXPECT_TRUE(V != nullptr);<br>
<br>
Either use:<br>
<br>
EXPECT_NE(nullptr, V);<br>
<br>
Or, my preference:<br>
<br>
EXPECT_TRUE(V);<br>
<br>
> + EXPECT_TRUE(V->getType()->isDoubleTy());<br>
> + EXPECT_TRUE(isa<ConstantFP>(V));<br>
> + EXPECT_TRUE(cast<ConstantFP>(V)->isExactlyValue(3.5));<br>
<br>
This will assert out and crash if `V` isn't a `ConstantFP`. Maybe the<br>
previous check for `isa` should be `ASSERT_TRUE` so that the test is<br>
aborted on failure without hiding results from other tests.<br>
<br>
> +<br>
> + V = parseTypeAndValue("i32 42", Error, M);<br>
> + EXPECT_TRUE(V != nullptr);<br>
> + EXPECT_TRUE(V->getType()->isIntegerTy());<br>
> + EXPECT_TRUE(isa<ConstantInt>(V));<br>
> + EXPECT_TRUE(cast<ConstantInt>(V)->equalsInt(42));<br>
> +<br>
> + V = parseTypeAndValue("duble 3.25", Error, M);<br>
> + EXPECT_FALSE(V != nullptr);<br>
<br>
This double negative is confusing. Here you're not even using the<br>
result `V`, so IMO you should just say:<br>
<br>
EXPECT_FALSE(parseTypeAndValue(...));<br>
<br>
Also, should you check `Error`?<br>
<br>
> +<br>
> + V = parseTypeAndValue("i32 3.25", Error, M);<br>
> + EXPECT_FALSE(V != nullptr);<br>
<br>
Should you check `Error`?<br>
<br>
> +<br>
> + V = parseTypeAndValue("i32 3, ", Error, M);<br>
> + EXPECT_FALSE(V != nullptr);<br>
> + EXPECT_EQ(Error.getMessage(), "expected end of string");<br>
> +}<br>
> +<br>
> } // end anonymous namespace<br>
><br>
<br>
<br>
</blockquote></div><br></div></div>