Skip to content
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.

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

Link against CIRCT and use it for codegen #234

Merged
merged 52 commits into from
Jan 16, 2022
Merged

Link against CIRCT and use it for codegen #234

merged 52 commits into from
Jan 16, 2022

Conversation

fabianschuiki
Copy link
Owner

This PR makes Moore link against the circt library and uses that to generate code.

More specifically, this adds a circt-sys crate with the low-level bindings against the MLIR C library (generated by the bindgen crate), and a circt crate which contains some higher-level wrapper code that makes CIRCT and MLIR easier to use from Rust code. Note that we do not establish any kind of safety in Rust terms, but the API is "safe enough" for what we're trying to use it for.

Currently codegen.rs calls the original LLHD crate and the new CIRCT bindings in parallel to generate code. This was useful as a sanity check and to make sure the CIRCT-based emission is up to par with the LLHD crate. At a later point we should strip codegen down to use only CIRCT.

This PR also adds a --format=mlir-native output option which uses MLIR's dialect printer to emit the assembly.

Add a first bindgen-based `circt-sys` crate that allows Rust code to use
the CIRCT and, by extension, MLIR project code.
Add a `circt` crate that contains a lightweight wrapper to make
`circt-sys` more comfortable to use from the Rust world. Add this crate
as a dependency of `svlog`, for later use in module emission.
@fabianschuiki fabianschuiki added L-vlog Language: Verilog and SystemVerilog. C-enhancement Category: Adding or improving on features. A-codegen Area: Code generation. labels Nov 21, 2021
@fabianschuiki
Copy link
Owner Author

fabianschuiki commented Nov 21, 2021

@maerhart I'd love to get your feedback/review on this, since you also opened issue #232 😃

Copy link
Collaborator

@maerhart maerhart left a comment

Choose a reason for hiding this comment

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

Excellent work! This is a big step forward in terms of merging Moore with CIRCT.
I see you even incorporated FileCheck tests.

Just a few comments, mostly on index bitwidth and unique name checking.

Additionally, it could be useful to add some instructions to the README on how to build moore as it got quite a bit more complicated with CIRCT as a submodule (or did I miss some automation?).
E.g.,

git submodule update --init --recursive
mkdir -p circt/build
mkdir -p circt/llvm/build
cd circt/llvm/build
cmake ../llvm \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_INSTALL_PREFIX=../install \
    -DLLVM_BUILD_EXAMPLES=OFF \
    -DLLVM_ENABLE_ASSERTIONS=ON \
    -DLLVM_ENABLE_BINDINGS=OFF \
    -DLLVM_ENABLE_OCAMLDOC=OFF \
    -DLLVM_ENABLE_PROJECTS=mlir \
    -DLLVM_INSTALL_UTILS=ON \
    -DLLVM_OPTIMIZED_TABLEGEN=ON \
    -DLLVM_TARGETS_TO_BUILD=""
cmake --build . --target install
cd ../../build
cmake .. -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_INSTALL_PREFIX=../install \
    -DMLIR_DIR=$PWD/../llvm/install/lib/cmake/mlir \
    -DLLVM_DIR=$PWD/../llvm/install/lib/cmake/llvm \
    -DLLVM_ENABLE_ASSERTIONS=ON
cmake --build . --target install
cd ../..
export CIRCT_SYS_CIRCT_DIR=`pwd`/circt
export CIRCT_SYS_CIRCT_BUILD_DIR=`pwd`/circt/install
export CIRCT_SYS_LLVM_DIR=`pwd`/circt/llvm
export CIRCT_SYS_LLVM_BUILD_DIR=`pwd`/circt/llvm/install
cargo build

.github/build-circt.sh Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
src/circt-sys/build.rs Outdated Show resolved Hide resolved
src/circt/src/comb.rs Outdated Show resolved Hide resolved
offset: usize,
length: usize,
) -> Self {
let offset = crate::hw::ConstantOp::new(builder, 64, &offset.into()).into();
Copy link
Collaborator

Choose a reason for hiding this comment

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

The IR emitted here will fail during the CIRCT verification pass because ArraySliceOp expects the bitwidth of offset to be llvm::Log2_64_Ceil(type_cast<ArrayType>($_self).getSize()).

Copy link
Owner Author

Choose a reason for hiding this comment

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

That's an excellent catch! I should have added a run of the verifier into Moore directly, just after codegen. Let me do that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that's a good idea!

src/svlog/codegen.rs Outdated Show resolved Hide resolved
self.builder.ins().sig(v)
(
self.builder.ins().sig(v.0),
circt::llhd::SignalOp::new(self.mlir_builder, "", v.1).into(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

circt::llhd::VariableOp::new(self.mlir_builder, inner).into()
} else if circt::llhd::is_signal_type(ty) {
let inner = self.emit_zero_for_type_mlir(circt::llhd::signal_type_element(ty));
circt::llhd::SignalOp::new(self.mlir_builder, "", inner).into()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

let init = self.emit_zero_for_type_both(ty);
let sig = (
self.builder.ins().sig(init.0),
circt::llhd::SignalOp::new(self.mlir_builder, "", init.1).into(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above.

self.builder.ins().br_cond(event_cond, init_blk, event_blk);
self.builder.append_to(event_blk);
let event_blk = self.mk_block(Some("event"));
self.mk_cond_br(event_cond, init_blk, event_blk);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the only call to mk_cond_br where the successors are not transposed. Are you sure that init_blk and event_blk shouldn't be transposed?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Ufff that would have been annoying to debug. Thanks a lot! 😬

@fabianschuiki
Copy link
Owner Author

Thanks a lot for the extremely detailed review @maerhart! This is awesome, and you caught quite a few bugs 😬.

Additionally, it could be useful to add some instructions to the README on how to build moore as it got quite a bit more complicated with CIRCT as a submodule (or did I miss some automation?).

Yeah that is a good idea. I'll add the snippet to the README. I was considering making the circt-sys crate fetch and build LLVM and CIRCT automatically if you don't provide the environment variables, but not sure if a 2 hour cargo check is what the user would expect. Maybe it's easier to just provide good instructions to the user, as you suggest?

@maerhart
Copy link
Collaborator

Maybe it's easier to just provide good instructions to the user, as you suggest?

I'd say good instructions are good enough for now. Automatic fetch and build is certainly more convenient from a user perspective (provided it always succeeds), but it should definitely print in the CLI when it does so and report the progress, otherwise one might think it ran into an infinite loop or so 😆

@fabianschuiki fabianschuiki merged commit f1aaa9c into master Jan 16, 2022
@fabianschuiki fabianschuiki deleted the mlir branch January 16, 2022 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation. C-enhancement Category: Adding or improving on features. L-vlog Language: Verilog and SystemVerilog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants