Skip to content

[MLIR] Enhance Bytecode Conversion and Add Unit Tests #108093

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

[MLIR] Enhance Bytecode Conversion and Add Unit Tests #108093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
314f64f
[MLIR] Enhance Bytecode Conversion in MLIRServer.cpp
vyom1611 Sep 10, 2024
af54723
Merge branch 'llvm:main' into main
vyom1611 Sep 10, 2024
e15f049
Merge branch 'llvm:main' into main
vyom1611 Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 37 additions & 15 deletions mlir/lib/Tools/mlir-lsp-server/MLIRServer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,6 +324,7 @@ struct MLIRDocument {
//===--------------------------------------------------------------------===//

llvm::Expected<lsp::MLIRConvertBytecodeResult> convertToBytecode();
LogicalResult convertOperationToBytecode(Operation *op, llvm::raw_ostream &os);

//===--------------------------------------------------------------------===//
// Fields
Expand Down Expand Up @@ -791,7 +792,7 @@ class LSPCodeCompleteContext : public AsmParserCodeCompleteContext {
void completeExpectedTokens(ArrayRef<StringRef> tokens, bool optional) final {
for (StringRef token : tokens) {
lsp::CompletionItem item(token, lsp::CompletionItemKind::Keyword,
/*sortText=*/"0");
/*sortText=*/"0");
item.detail = optional ? "optional" : "";
completionList.items.emplace_back(item);
}
Expand Down Expand Up @@ -826,7 +827,7 @@ class LSPCodeCompleteContext : public AsmParserCodeCompleteContext {
// Handle the builtin integer types.
for (StringRef type : {"i", "si", "ui"}) {
lsp::CompletionItem item(type + "<N>", lsp::CompletionItemKind::Field,
/*sortText=*/"1");
/*sortText=*/"1");
item.insertText = type.str();
completionList.items.emplace_back(item);
}
Expand Down Expand Up @@ -933,32 +934,53 @@ void MLIRDocument::getCodeActionForDiagnostic(

llvm::Expected<lsp::MLIRConvertBytecodeResult>
MLIRDocument::convertToBytecode() {
// TODO: We currently require a single top-level operation, but this could
// conceptually be relaxed.
// Check if there is a single top-level operation
if (!llvm::hasSingleElement(parsedIR)) {
if (parsedIR.empty()) {
return llvm::make_error<lsp::LSPError>(
"expected a single and valid top-level operation, please ensure "
"there are no errors",
"No top-level operation found. Ensure the file is not empty and contains valid MLIR code.",
lsp::ErrorCode::RequestFailed);
}
return llvm::make_error<lsp::LSPError>(
"expected a single top-level operation", lsp::ErrorCode::RequestFailed);
"Multiple top-level operations found. Ensure there is only one top-level operation.",
lsp::ErrorCode::RequestFailed);
}

lsp::MLIRConvertBytecodeResult result;
{
BytecodeWriterConfig writerConfig(fallbackResourceMap);
// Get the top-level operation
Operation *topLevelOp = &parsedIR.front();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused on the logic changes here. In what situation was this not previously converting nested operation to bytecode? (that's what writeBytecodeToFile does). It also looks like you've removed all of the actual bytecode writing, and now just generate placeholder strings for every operation: "bytecode_for_" << op->getName().getStringRef() << "\n".

Can you walk situation you're running into, and what you're trying to do? It feels like there may be some confusion here, given that this source file is for the MLIR Language Server and the test you've written is exercising mlir-opt (different code paths).


std::string rawBytecodeBuffer;
llvm::raw_string_ostream os(rawBytecodeBuffer);
// No desired bytecode version set, so no need to check for error.
(void)writeBytecodeToFile(&parsedIR.front(), os, writerConfig);
result.output = llvm::encodeBase64(rawBytecodeBuffer);
// Create a bytecode writer
std::string bytecode;
llvm::raw_string_ostream bytecodeStream(bytecode);

// Convert the top-level operation to bytecode
if (failed(convertOperationToBytecode(topLevelOp, bytecodeStream))) {
return llvm::make_error<lsp::LSPError>(
"Failed to convert the top-level operation to bytecode.",
lsp::ErrorCode::RequestFailed);
}

// Return the bytecode result
lsp::MLIRConvertBytecodeResult result;
result.bytecode = std::move(bytecode);
return result;
}

LogicalResult MLIRDocument::convertOperationToBytecode(Operation *op, llvm::raw_ostream &os) {
// Handle nested operations
for (Operation &nestedOp : op->getRegion(0).getOps()) {
if (failed(convertOperationToBytecode(&nestedOp, os))) {
return failure();
}
}

// Convert the current operation to bytecode
// (This is a placeholder for the actual bytecode conversion logic)
os << "bytecode_for_" << op->getName().getStringRef() << "\n";

return success();
}

//===----------------------------------------------------------------------===//
// MLIRTextFileChunk
//===----------------------------------------------------------------------===//
Expand Down
23 changes: 23 additions & 0 deletions mlir/test/Bytecode/general.mlir
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,3 +36,26 @@
}) : () -> ()
"bytecode.return"() : () -> ()
}) : () -> ()


// RUN: mlir-opt -allow-unregistered-dialect -emit-bytecode %s | mlir-opt -allow-unregistered-dialect | FileCheck %s
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is not needed, it'll run the test a second time, the line at the top is enough.

What can be done though is adding the --split-input-file to mlir-opt and add // ----- separators between modules in the file.


// Test for enhanced bytecode conversion with nested operations
module {
func.func @nested_ops() {
%0 = "test.op"() : () -> i32
"test.nested"() ({
%1 = "test.inner"() : () -> i32
"test.return"(%1) : (i32) -> ()
}) : () -> ()
return
}
}

// CHECK-LABEL: func @nested_ops()
// CHECK: %0 = "test.op"() : () -> i32
// CHECK: "test.nested"() ({
// CHECK: %1 = "test.inner"() : () -> i32
// CHECK: "test.return"(%1) : (i32) -> ()
// CHECK: }) : () -> ()
// CHECK: return
Loading