Gemmini: integrate dialect and target plugin (matmul lowering)#12
Gemmini: integrate dialect and target plugin (matmul lowering)#12
Conversation
There was a problem hiding this comment.
Pull request overview
Integrates the Gemmini MLIR dialect + lowering passes into Merlin as an IREE compiler target plugin, including a lit test that exercises matmul lowering through to emitted RISC-V Gemmini intrinsics.
Changes:
- Adds Gemmini dialect IR, lowering passes (Linalg→Gemmini, Gemmini→LLVM intrinsics), and IR dump pass under
compiler/src/merlin/Dialect/Gemmini. - Wires up the Gemmini plugin registration/build under
compiler/plugins/target/Gemminiand enables it viairee_compiler_plugin.cmake. - Adds a Gemmini matmul lit test.
Reviewed changes
Copilot reviewed 28 out of 33 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| iree_compiler_plugin.cmake | Enables external Gemmini plugin + adds include paths and builds plugin subdir. |
| compiler/src/merlin/Dialect/Gemmini/_generated/merlin/Dialect/Gemmini/IR/GemminiDialect.h.inc | Generated Gemmini dialect declaration. |
| compiler/src/merlin/Dialect/Gemmini/_generated/merlin/Dialect/Gemmini/IR/GemminiDialect.cpp.inc | Generated Gemmini dialect definition/type ID. |
| compiler/src/merlin/Dialect/Gemmini/_generated/GemminiConversions.inc | Generated op→LLVM intrinsic conversion glue. |
| compiler/src/merlin/Dialect/Gemmini/Transforms/LowerLinalgToGemmini.cpp | New pass/patterns lowering selected Linalg ops to Gemmini tile ops. |
| compiler/src/merlin/Dialect/Gemmini/Transforms/LowerGemminiPass.cpp | New pass lowering Gemmini ops to LLVM intrinsics / C-stub calls; includes Print lowering. |
| compiler/src/merlin/Dialect/Gemmini/Transforms/LegalizeForLLVMExport.cpp | Legalizes Gemmini ops for LLVM export + rewrites to Gemmini intrinsic ops. |
| compiler/src/merlin/Dialect/Gemmini/Transforms/GemminiIRDumps.cpp | Adds IR dumping pass for Gemmini-related ops. |
| compiler/src/merlin/Dialect/Gemmini/Transforms/CMakeLists.txt | Builds Gemmini transforms as an iree_cc_library. |
| compiler/src/merlin/Dialect/Gemmini/RegisterGemmini.h | Declares dialect + pass registration and pass factory helpers. |
| compiler/src/merlin/Dialect/Gemmini/RegisterGemmini.cpp | Implements dialect/pass registration and pass factories. |
| compiler/src/merlin/Dialect/Gemmini/IR/GemminiOps.h | Updates Gemmini ops header includes (LLVM dialect). |
| compiler/src/merlin/Dialect/Gemmini/IR/Gemmini.td | Updates dialect namespace + defines additional intrinsic ops. |
| compiler/src/merlin/Dialect/Gemmini/IR/CMakeLists.txt | Builds Gemmini dialect IR as an iree_cc_library. |
| compiler/src/merlin/Dialect/Gemmini/CMakeLists.txt | Builds Gemmini registration library as an iree_cc_library. |
| compiler/plugins/target/Gemmini/test/lit.cfg.py | Lit config for Gemmini plugin tests. |
| compiler/plugins/target/Gemmini/test/gemmini_matmul_lowering.mlir | Lit test for matmul lowering to Gemmini intrinsics. |
| compiler/plugins/target/Gemmini/test/Output/gemmini_matmul_lowering.mlir.tmp.dir/module_matmul_dispatch_0_embedded_elf_riscv_64_benchmark.mlir | Generated test output artifact (should not be committed). |
| compiler/plugins/target/Gemmini/test/Output/gemmini_matmul_lowering.mlir.tmp.dir/module_matmul_dispatch_0_embedded_elf_riscv_64.optimized.bc | Generated test output artifact (should not be committed). |
| compiler/plugins/target/Gemmini/test/Output/gemmini_matmul_lowering.mlir.tmp.dir/module_matmul_dispatch_0_embedded_elf_riscv_64.linked.bc | Generated test output artifact (should not be committed). |
| compiler/plugins/target/Gemmini/test/Output/gemmini_matmul_lowering.mlir.tmp.dir/module_matmul_dispatch_0_embedded_elf_riscv_64.codegen.ll | Generated test output artifact (should not be committed). |
| compiler/plugins/target/Gemmini/test/Output/gemmini_matmul_lowering.mlir.tmp.dir/module_matmul_dispatch_0_embedded_elf_riscv_64.codegen.bc | Generated test output artifact (should not be committed). |
| compiler/plugins/target/Gemmini/test/Output/gemmini_matmul_lowering.mlir.tmp.dir/module_matmul_dispatch_0.mlir | Generated test output artifact (should not be committed). |
| compiler/plugins/target/Gemmini/test/Output/gemmini_matmul_lowering.mlir.tmp.dir/configured_module_matmul_dispatch_0.mlir | Generated test output artifact (should not be committed). |
| compiler/plugins/target/Gemmini/test/Output/gemmini_matmul_lowering.mlir.script | Generated test output artifact (should not be committed). |
| compiler/plugins/target/Gemmini/test/.lit_test_times.txt | Generated test timing artifact (should not be committed). |
| compiler/plugins/target/Gemmini/PluginRegistration.cpp | Simplifies plugin session to register dialect/passes and uses DefaultActivated policy. |
| compiler/plugins/target/Gemmini/CMakeLists.txt | Builds dialect/passes/registration and registers plugin; adds lit suite. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| set(_GEMMINI_SRC_DIR "${MERLIN_COMPILER_SOURCE_DIR}/src/merlin/Dialect/Gemmini") | ||
|
|
||
| # --- Gemmini Dialect IR Library --- | ||
| iree_cc_library( | ||
| NAME | ||
| IR | ||
| SRCS | ||
| "${_GEMMINI_SRC_DIR}/IR/GemminiDialect.cpp" | ||
| DEPS | ||
| MLIRFuncDialect | ||
| MLIRIR | ||
| MLIRLLVMDialect | ||
| MLIRSideEffectInterfaces | ||
| PUBLIC | ||
| ) |
There was a problem hiding this comment.
These iree_cc_library targets compile sources that include headers from the Merlin source tree and the pre-generated _generated/ directory, but this CMakeLists doesn't add any target-scoped include directories. Right now it relies on the global include_directories(...) from iree_compiler_plugin.cmake, which makes the plugin harder to build/reuse in isolation.
Consider adding the required include paths via target-scoped includes (e.g. an INCLUDES/HDRS mechanism in iree_cc_library or target_include_directories) for ::IR/::Transforms instead of depending on global include state.
| Value input0 = inputs[0]; | ||
| Value input1 = inputs[1]; | ||
| Value output0 = ouputs[0]; | ||
| MemRefType input0Type = dyn_cast<MemRefType>(input0.getType()); |
There was a problem hiding this comment.
MatmulLowering assumes all operands are memref-typed and immediately dereferences input0Type (input0Type.getShape()). If linalg.matmul is still tensor-based (common pre-bufferization in IREE), dyn_cast<MemRefType> returns null and this will crash the compiler.
Add a type check (and emit notifyMatchFailure) before using the shapes, or explicitly restrict the pass pipeline so it only runs after bufferization/buffer semantics are guaranteed.
| MemRefType input0Type = dyn_cast<MemRefType>(input0.getType()); | |
| MemRefType input0Type = dyn_cast<MemRefType>(input0.getType()); | |
| if (!input0Type) { | |
| return rewriter.notifyMatchFailure( | |
| matMulOp, "MatmulLowering expects memref-typed operands"); | |
| } |
| # Include paths so that #include "merlin/Dialect/Gemmini/..." resolves | ||
| # from the source tree AND the pre-generated .inc files. | ||
| include_directories( | ||
| "${MERLIN_COMPILER_SOURCE_DIR}/src" | ||
| "${MERLIN_COMPILER_SOURCE_DIR}/src/merlin/Dialect/Gemmini/_generated" | ||
| ) |
There was a problem hiding this comment.
include_directories(...) here sets global include paths for the entire IREE build when this plugin is enabled. That can mask missing includes, change header resolution order, and cause subtle ODR/build issues.
Prefer adding these include paths at the target level (e.g. via the iree_cc_library include/INCLUDES mechanism or target_include_directories on the Gemmini targets) so they only apply to the plugin’s libraries.
| @@ -0,0 +1 @@ | |||
| 2.201907e+00 gemmini_matmul_lowering.mlir | |||
There was a problem hiding this comment.
.lit_test_times.txt is a generated file produced by lit test runs and is environment-dependent. It generally shouldn't be committed to the repository; consider removing it and adding it to .gitignore to keep diffs clean.
| 2.201907e+00 gemmini_matmul_lowering.mlir | |
| # Intentionally left empty; lit test timing data is generated and should not be committed. |
| // Override explicitly to allow conditional dialect dependence. | ||
| void getDependentDialects(DialectRegistry ®istry) const override { | ||
| registry.insert<LLVM::LLVMDialect>(); | ||
| registry.insert<LLVM::LLVMDialect>(); |
There was a problem hiding this comment.
getDependentDialects inserts LLVM::LLVMDialect twice. This is harmless but redundant; consider removing the duplicate line to keep the dependency list clear.
| registry.insert<LLVM::LLVMDialect>(); |
| auto ouputs = matMulOp.getOutputs(); | ||
| Location loc = matMulOp.getLoc(); | ||
| Value input0 = inputs[0]; | ||
| Value input1 = inputs[1]; | ||
| Value output0 = ouputs[0]; |
There was a problem hiding this comment.
Typo: local variable ouputs is misspelled (should be outputs). This is a new identifier and makes the code harder to scan/search.
| auto ouputs = matMulOp.getOutputs(); | |
| Location loc = matMulOp.getLoc(); | |
| Value input0 = inputs[0]; | |
| Value input1 = inputs[1]; | |
| Value output0 = ouputs[0]; | |
| auto outputs = matMulOp.getOutputs(); | |
| Location loc = matMulOp.getLoc(); | |
| Value input0 = inputs[0]; | |
| Value input1 = inputs[1]; | |
| Value output0 = outputs[0]; |
| def Gemmini_ConifgLd_IntrOp : Gemmini_IntrOpBase<"config_ld">, | ||
| Arguments<(ins LLVM_Type, LLVM_Type)>; | ||
|
|
||
| def Gemmini_ConfigSt_IntrOp : Gemmini_IntrOpBase<"config_st">, | ||
| Arguments<(ins LLVM_Type, LLVM_Type)>; | ||
|
|
There was a problem hiding this comment.
The intrinsic op name Gemmini_ConifgLd_IntrOp appears to be misspelled (Conifg vs Config). Since this becomes part of the C++ op class name (and is referenced from multiple passes), it's worth fixing now to avoid a permanent API typo.
| # --- Gemmini Plugin --- | ||
| # Disable the in-tree gemmini plugin (from the IREE fork) in favour of | ||
| # the one provided here in Merlin. | ||
| set(IREE_GEMMINI_EXTERNAL_PLUGIN ON CACHE BOOL "" FORCE) | ||
|
|
||
| # --- Target Plugins --- | ||
| # We use the same flags defined in build.py and the runtime plugin. | ||
| # Include paths so that #include "merlin/Dialect/Gemmini/..." resolves | ||
| # from the source tree AND the pre-generated .inc files. | ||
| include_directories( | ||
| "${MERLIN_COMPILER_SOURCE_DIR}/src" | ||
| "${MERLIN_COMPILER_SOURCE_DIR}/src/merlin/Dialect/Gemmini/_generated" | ||
| ) | ||
|
|
||
| # 1. SpacemiT X60 Support | ||
| if(MERLIN_BUILD_SPACEMITX60) | ||
| if(NOT MERLIN_ENABLE_CORE) | ||
| message(FATAL_ERROR "MERLIN_BUILD_SPACEMITX60 requires MERLIN_ENABLE_CORE=ON") | ||
| endif() | ||
|
|
||
| if(EXISTS "${MERLIN_COMPILER_SOURCE_DIR}/plugins/target/SpacemiT/CMakeLists.txt") | ||
| add_subdirectory("${MERLIN_COMPILER_SOURCE_DIR}/plugins/target/SpacemiT" | ||
| "${MERLIN_COMPILER_BINARY_ROOT}/target/SpacemiT") | ||
| endif() | ||
| endif() | ||
|
|
||
| # 2. Saturn OPU Support | ||
| if(MERLIN_BUILD_SATURN_OPU) | ||
| if(NOT MERLIN_ENABLE_CORE) | ||
| message(FATAL_ERROR "MERLIN_BUILD_SATURN_OPU requires MERLIN_ENABLE_CORE=ON") | ||
| endif() | ||
|
|
||
| if(EXISTS "${MERLIN_COMPILER_SOURCE_DIR}/plugins/target/Saturn/CMakeLists.txt") | ||
| add_subdirectory("${MERLIN_COMPILER_SOURCE_DIR}/plugins/target/Saturn" | ||
| "${MERLIN_COMPILER_BINARY_ROOT}/target/Saturn") | ||
| endif() | ||
| endif() | ||
|
|
||
| # 3. Gemmini Support | ||
| # If Gemmini is still used by specific targets (like FireSim), | ||
| # you can gate it with MERLIN_BUILD_GEMMINI or a similar flag. | ||
| if(MERLIN_BUILD_GEMMINI) | ||
| add_subdirectory("${MERLIN_COMPILER_SOURCE_DIR}/plugins/target/Gemmini" | ||
| "${MERLIN_COMPILER_BINARY_ROOT}/target/Gemmini") | ||
| endif() No newline at end of file | ||
| # Build the Gemmini plugin (dialect + passes + registration) in one package. | ||
| add_subdirectory( | ||
| "${MERLIN_COMPILER_SOURCE_DIR}/plugins/target/Gemmini" | ||
| "${MERLIN_COMPILER_BINARY_ROOT}/target/Gemmini" | ||
| ) No newline at end of file |
There was a problem hiding this comment.
add_subdirectory for Gemmini is unconditional, so -DMERLIN_ENABLE_TARGET_GEMMINI=ON/OFF (used by tools/build.py) no longer has any effect. If this is intended to be optional, gate the plugin build on the flag (and consider keeping the previous fatal-error checks for required dependencies).
| @iree_hal_executable_library_query_v0_attrs = private constant [1 x %iree_hal_executable_dispatch_attrs_v0_t] [%iree_hal_executable_dispatch_attrs_v0_t { i64 0, i16 0, i8 0, i8 3, i32 1, i32 1, i16 1, i16 0, i64 0, i64 0, i64 0, i64 0, i64 0 }] | ||
| @3 = private constant [35 x i8] c"matmul_dispatch_0_matmul_4x4x4_f32\00", align 1 | ||
| @iree_hal_executable_library_query_v0_names = private constant [1 x ptr] [ptr @3] | ||
| @4 = private constant [153 x i8] c"/Users/sparshsingh/work/merlin/compiler/plugins/target/Gemmini/test/Output/gemmini_matmul_lowering.mlir.tmp.dir/configured_module_matmul_dispatch_0.mlir\00", align 1 |
There was a problem hiding this comment.
This file appears to be a generated test artifact (LLVM IR dumped by the lit RUN pipeline) and it embeds an absolute path from the local machine (/Users/...). These test/Output/... and *.tmp.dir/* artifacts should not be committed (they are non-hermetic, noisy in diffs, and can leak local path/user info).
Recommend deleting the committed Output directory contents and ensuring lit writes to a temporary directory only (plus add a .gitignore rule to prevent re-adding them).
| @4 = private constant [153 x i8] c"/Users/sparshsingh/work/merlin/compiler/plugins/target/Gemmini/test/Output/gemmini_matmul_lowering.mlir.tmp.dir/configured_module_matmul_dispatch_0.mlir\00", align 1 | |
| @4 = private constant [65 x i8] c"configured_module_matmul_dispatch_0.mlir\00", align 1 |
| } else if (memElementType == rewriter.getI8Type() || | ||
| memElementType == rewriter.getI32Type()) { | ||
| formatSpecifierCst = getOrCreateGlobalString( | ||
| loc, rewriter, "frmt_spec", StringRef("%d \0", 4), parentModule); |
There was a problem hiding this comment.
formatSpecifierCst is only assigned for f32/f64 and i8/i32 element types. For other element types (e.g. i16/i64, which are otherwise supported by the pass options), it remains uninitialized and is still used in the final LLVM::CallOp, which is undefined behavior.
Handle the default case (emit a match failure / pass error) or initialize formatSpecifierCst for all supported element types.
| loc, rewriter, "frmt_spec", StringRef("%d \0", 4), parentModule); | |
| loc, rewriter, "frmt_spec", StringRef("%d \0", 4), parentModule); | |
| } else { | |
| return rewriter.notifyMatchFailure( | |
| op, "unsupported memref element type in Gemmini print lowering"); |
|
I would suggest that we don't include all the generated files (e.g.: *.mlir, *.s, etc) These files can be generated by the user. I would still suggest to have a /test/ folder in the corresponding Gemmini dialect folder to test this. The correct to use this is either with mlir-lit or iree-lit Example on how to write those test is this -> LINK TO RELEVANT TEST |
|
Also lets not include h.inc files. Think it from an user perspective :) What can they generate themselves automatically. Everything that we expose for them to generate we shouldnt include |
- Remove pre-generated .inc files and test Output/ artifacts from git - Use iree_tablegen_library to generate .inc from Gemmini.td at build time - Move lit test to compiler/src/merlin/Dialect/Gemmini/test/ - Add .gitignore for generated artifacts (_generated/, *.inc, Output/) - Simplify lit.cfg.py (iree_lit_test handles tool paths) - Update iree_compiler_plugin.cmake to drop _generated include path - Add -DIREE_GEMMINI_EXTERNAL_PLUGIN=ON to tools/build.py
Summary
• Wire Gemmini as a Merlin target plugin under compiler/plugins/target/Gemmini.
• Hook Gemmini dialect + passes under compiler/src/merlin/Dialect/Gemmini (IR + LowerLinalgToGemmini + LowerGemmini + LegalizeForLLVMExport + IR dumps).
• Use iree_compiler_register_plugin(merlin_gemmini) and MERLIN_ENABLE_TARGET_GEMMINI cmake flag.
• Keep all IREE/LLVM changes routed through third_party/iree_bar as build.py expects (no direct edits to external forks here).