[mlir:Parser] Always splice parsed operations to the end of the parsed block

[rriddle]

Summary

The current splicing behavior dates back to when all blocks had terminators,
so we would "helpfully" splice before the terminator. This doesn't make sense
anymore, and leads to somewhat unexpected results when parsing multiple
pieces of IR into the same block.

[bzcheeseman]

LGTM, thanks River :)

mlir/lib/AsmParser/Parser.cpp

while (true) {
     case Token::eof: {
       if (opParser.finalize())
         return failure();
 
       // Splice the blocks of the parsed operation over to the provided
       // top-level block.
       auto &parsedOps = topLevelOp->getBody()->getOperations();
       auto &destOps = topLevelBlock->getOperations();
-      destOps.splice(destOps.empty() ? destOps.end() : std::prev(destOps.end()),
-                     parsedOps, parsedOps.begin(), parsedOps.end());
+      destOps.splice(destOps.end(), parsedOps, parsedOps.begin(),
+                     parsedOps.end());
       return success();
     }
 
     // If we got an error token, then the lexer already emitted an error, just
     // stop.  Someday we could introduce error recovery if there was demand
     // for it.
     case Token::error:
       return failure();

mlir/lib/Bytecode/Reader/BytecodeReader.cpp

LogicalResult BytecodeReader::parseIRSection(ArrayRef<uint8_t> sectionData,
 
   // Verify that the parsed operations are valid.
   if (config.shouldVerifyAfterParse() && failed(verify(*moduleOp)))
     return failure();
 
   // Splice the parsed operations over to the provided top-level block.
   auto &parsedOps = moduleOp->getBody()->getOperations();
   auto &destOps = block->getOperations();
-  destOps.splice(destOps.empty() ? destOps.end() : std::prev(destOps.end()),
-                 parsedOps, parsedOps.begin(), parsedOps.end());
+  destOps.splice(destOps.end(), parsedOps, parsedOps.begin(), parsedOps.end());
   return success();
 }
 
 LogicalResult
 BytecodeReader::parseRegions(EncodingReader &reader,
                              std::vector<RegionReadState> &regionStack,
                              RegionReadState &readState) {
   // Read the regions of this operation.

mlir/unittests/Parser/ParserTest.cpp

TEST(MLIRParser, ParseInvalidIR) {
   MLIRContext context;
   ParserConfig config(&context, /*verifyAfterParse=*/false);
 
   // Check that we properly parse the op, but it fails the verifier.
   OwningOpRef<ModuleOp> module = parseSourceString<ModuleOp>(moduleStr, config);
   ASSERT_TRUE(module);
   ASSERT_TRUE(failed(verify(*module)));
 }
+
+TEST(MLIRParser, ParseAtEnd) {
+  std::string firstModuleStr = R"mlir(
+    "test.first"() : () -> ()
+  )mlir";
+  std::string secondModuleStr = R"mlir(
+    "test.second"() : () -> ()
+  )mlir";
+
+  MLIRContext context;
+  context.allowUnregisteredDialects();
+  Block block;
+
+  // Parse the first module string.
+  LogicalResult firstParse =
+      parseSourceString(firstModuleStr, &block, &context);
+  EXPECT_TRUE(succeeded(firstParse));
+
+  // Parse the second module string.
+  LogicalResult secondParse =
+      parseSourceString(secondModuleStr, &block, &context);
+  EXPECT_TRUE(succeeded(secondParse));
+
+  // Check the we parse at the end.
+  EXPECT_EQ(block.front().getName().getStringRef(), "test.first");
+  EXPECT_EQ(block.back().getName().getStringRef(), "test.second");
+}
 } // namespace