From f357780c33c3bc721ee3e5903cfeefd02c5ce329 Mon Sep 17 00:00:00 2001 From: "Rickert, Jonas" Date: Tue, 20 Aug 2024 12:45:42 +0100 Subject: [PATCH 1/3] [mlir] Do not set lastToken in AsmParser's resetToken function The member lastToken is the last token that was consumed by the parser. Resetting the lexer position to a different position does not cause any token to be consumed, so lastToken should not be updated. Setting it to curToken can cause the scopeLoc.end location of OperationDefinition to be off-by-one, pointing to the first token after the operation. An example for an operation for which the scopeLoc.end location was wrong before is: %0 = torch.vtensor.literal(dense_resource<__elided__> : tensor<768xbf16>) : !torch.vtensor<[768],bf16> . Here the scope end loc always pointed to the next token --- mlir/lib/AsmParser/Parser.h | 1 - 1 file changed, 1 deletion(-) diff --git a/mlir/lib/AsmParser/Parser.h b/mlir/lib/AsmParser/Parser.h index b959e67b8e2583..e6051b0e21eac9 100644 --- a/mlir/lib/AsmParser/Parser.h +++ b/mlir/lib/AsmParser/Parser.h @@ -133,7 +133,6 @@ class Parser { /// Reset the parser to the given lexer position. void resetToken(const char *tokPos) { state.lex.resetPointer(tokPos); - state.lastToken = state.curToken; state.curToken = state.lex.lexToken(); } From 42dbd0942e490faef4707f5815a537858a572495 Mon Sep 17 00:00:00 2001 From: "Rickert, Jonas" Date: Wed, 21 Aug 2024 10:56:50 +0100 Subject: [PATCH 2/3] Add a unit test for AsmParser's file locations This test checks the parsed ops' locations and scope locations. It uses the !llvm.array type in the test to check that locations are correct for ops that contain an extended-type. The parser for extended-types calls resetToken, which caused wrong locations in the past. --- mlir/unittests/Parser/CMakeLists.txt | 2 + mlir/unittests/Parser/ParserTest.cpp | 81 ++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/mlir/unittests/Parser/CMakeLists.txt b/mlir/unittests/Parser/CMakeLists.txt index f5646ff3f2c345..a5e2da56ffb57e 100644 --- a/mlir/unittests/Parser/CMakeLists.txt +++ b/mlir/unittests/Parser/CMakeLists.txt @@ -8,6 +8,8 @@ add_mlir_unittest(MLIRParserTests target_include_directories(MLIRParserTests PRIVATE "${MLIR_BINARY_DIR}/test/lib/Dialect/Test") target_link_libraries(MLIRParserTests PRIVATE + MLIRFuncDialect + MLIRLLVMDialect MLIRIR MLIRParser MLIRTestDialect diff --git a/mlir/unittests/Parser/ParserTest.cpp b/mlir/unittests/Parser/ParserTest.cpp index 62f609ecf80492..52b965bcc1326b 100644 --- a/mlir/unittests/Parser/ParserTest.cpp +++ b/mlir/unittests/Parser/ParserTest.cpp @@ -8,8 +8,12 @@ #include "mlir/Parser/Parser.h" #include "mlir/AsmParser/AsmParser.h" +#include "mlir/AsmParser/AsmParserState.h" +#include "mlir/Dialect/Func/IR/FuncOps.h" +#include "mlir/Dialect/LLVMIR/LLVMDialect.h" #include "mlir/IR/BuiltinOps.h" #include "mlir/IR/Verifier.h" +#include "llvm/Support/SourceMgr.h" #include "gmock/gmock.h" @@ -101,4 +105,81 @@ TEST(MLIRParser, ParseAttr) { EXPECT_EQ(attr, b.getI64IntegerAttr(9)); } } + +TEST(MLIRParser, AsmParserLocations) { + std::string moduleStr = R"mlir( +func.func @foo() -> !llvm.array<2 x f32> { + %0 = llvm.mlir.undef : !llvm.array<2 x f32> + func.return %0 : !llvm.array<2 x f32> +} + )mlir"; + + DialectRegistry registry; + registry.insert(); + MLIRContext context(registry); + + auto memBuffer = + llvm::MemoryBuffer::getMemBuffer(moduleStr, "AsmParserTest.mlir", + /*RequiresNullTerminator=*/false); + ASSERT_TRUE(memBuffer); + + llvm::SourceMgr sourceMgr; + sourceMgr.AddNewSourceBuffer(std::move(memBuffer), llvm::SMLoc()); + + Block block; + AsmParserState parseState; + const LogicalResult parseResult = + parseAsmSourceFile(sourceMgr, &block, &context, &parseState); + ASSERT_TRUE(parseResult.succeeded()); + + auto funcOp = *block.getOps().begin(); + const AsmParserState::OperationDefinition *funcOpDefinition = + parseState.getOpDef(funcOp); + ASSERT_TRUE(funcOpDefinition); + + const std::pair expectedStartFunc{2u, 1u}; + const std::pair expectedEndFunc{2u, 10u}; + const std::pair expectedScopeEndFunc{5u, 2u}; + ASSERT_EQ(sourceMgr.getLineAndColumn(funcOpDefinition->loc.Start), + expectedStartFunc); + ASSERT_EQ(sourceMgr.getLineAndColumn(funcOpDefinition->loc.End), + expectedEndFunc); + ASSERT_EQ(funcOpDefinition->loc.Start, funcOpDefinition->scopeLoc.Start); + ASSERT_EQ(sourceMgr.getLineAndColumn(funcOpDefinition->scopeLoc.End), + expectedScopeEndFunc); + + auto llvmUndef = *funcOp.getOps().begin(); + const AsmParserState::OperationDefinition *llvmUndefDefinition = + parseState.getOpDef(llvmUndef); + ASSERT_TRUE(llvmUndefDefinition); + + const std::pair expectedStartUndef{3u, 8u}; + const std::pair expectedEndUndef{3u, 23u}; + const std::pair expectedScopeEndUndef{3u, 46u}; + ASSERT_EQ(sourceMgr.getLineAndColumn(llvmUndefDefinition->loc.Start), + expectedStartUndef); + ASSERT_EQ(sourceMgr.getLineAndColumn(llvmUndefDefinition->loc.End), + expectedEndUndef); + ASSERT_EQ(llvmUndefDefinition->loc.Start, + llvmUndefDefinition->scopeLoc.Start); + ASSERT_EQ(sourceMgr.getLineAndColumn(llvmUndefDefinition->scopeLoc.End), + expectedScopeEndUndef); + + auto funcReturn = *funcOp.getOps().begin(); + const AsmParserState::OperationDefinition *funcReturnDefinition = + parseState.getOpDef(funcReturn); + ASSERT_TRUE(funcReturnDefinition); + + const std::pair expectedStartReturn{4u, 3u}; + const std::pair expectedEndReturn{4u, 14u}; + const std::pair expectedScopeEndReturn{4u, 40u}; + ASSERT_EQ(sourceMgr.getLineAndColumn(funcReturnDefinition->loc.Start), + expectedStartReturn); + ASSERT_EQ(sourceMgr.getLineAndColumn(funcReturnDefinition->loc.End), + expectedEndReturn); + ASSERT_EQ(funcReturnDefinition->loc.Start, + funcReturnDefinition->scopeLoc.Start); + ASSERT_EQ(sourceMgr.getLineAndColumn(funcReturnDefinition->scopeLoc.End), + expectedScopeEndReturn); +} } // namespace From 5f083d4005b0d755c5b3f138f4a4023ce6b67f2c Mon Sep 17 00:00:00 2001 From: "Rickert, Jonas" Date: Wed, 21 Aug 2024 11:05:01 +0100 Subject: [PATCH 3/3] Add nodiscard attribute to allowsUnregisteredDialects This getter is can easily be confused with the similar named allowUnregisteredDialects setter --- mlir/include/mlir/IR/MLIRContext.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mlir/include/mlir/IR/MLIRContext.h b/mlir/include/mlir/IR/MLIRContext.h index 11e5329f43e681..d17bbac81655b5 100644 --- a/mlir/include/mlir/IR/MLIRContext.h +++ b/mlir/include/mlir/IR/MLIRContext.h @@ -133,7 +133,7 @@ class MLIRContext { Dialect *getOrLoadDialect(StringRef name); /// Return true if we allow to create operation for unregistered dialects. - bool allowsUnregisteredDialects(); + [[nodiscard]] bool allowsUnregisteredDialects(); /// Enables creating operations in unregistered dialects. /// This option is **heavily discouraged**: it is convenient during testing