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

[Moore] Make simple bit vectors a proper MLIR type #7011

Merged
merged 1 commit into from
May 9, 2024

Conversation

fabianschuiki
Copy link
Contributor

@fabianschuiki fabianschuiki commented May 9, 2024

The core type most SystemVerilog expressions are interested in and operate on is a "simple bit vector type". These are individual integer atoms like bit or logic, integral types like int, or packed arrays with a single dimension and an integer atom inner type, like bit [42:0]. So in a nutshell, simple bit vector types are MLIR's i42 in the two-valued (bit) case, or the four-valued equivalent (logic).

Up until this point, the Moore dialect reflected this pattern by providing an IntType for the integer atoms like bit and int, and using the PackedRangeDim for single dimension vectors of bit. A SimpleBitVectorType helper struct would then summarize the actual bit vector that was expressed by the packed range and integer atom. This makes working with the types in TableGen files very annoying, since the thing you are actually interested in -- the simple bit vector -- is not a proper MLIR type, but more like a helper struct on the side.

This commit rips out the existing IntType and its composition with a packed array dimension, and replaces it with a proper simple bit vector type that is actually an MLIR type. As a result, SystemVerilog types like int unsigned, bit [42:0], reg, logic signed [31:0], or integer are all translated into the same MLIR type. This new simple bit vector MLIR type retains the IntType name, and prints as !moore.i42 or !moore.l42, depending on whether it is a two-valued or four-valued integer. Single bit and logic atoms become i1 and l1 respectively.

This makes the Moore type system a lot easier to work with and removes a lot of unnecessary noise. Operations can now simply use llvm::isa<IntType> to check if a value is a simple bit vector.

@hailongSun2000
Copy link
Member

There are some outdated C API of moore remaining in the lib/CAPI/Dialect/Moore.cpp. Like IntType and SBVT

Copy link
Member

@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.

Let's say I have a !moore.packed<range<i1, 4:0>>. Would that be equivalent to !moore.i5?
Both bit and bit [0:0] correspond to !moore.i1, right? But is bit [0:0][1:0] considered a simple bitvector? Is there some canonicalization necessary or does Slang already canonicalize this?

include/circt/Dialect/Moore/MooreTypes.td Outdated Show resolved Hide resolved
lib/Conversion/MooreToCore/MooreToCore.cpp Show resolved Hide resolved
lib/Dialect/Moore/MooreTypes.cpp Show resolved Hide resolved
%arg5: !moore.unpacked<assoc<i1>>,
%arg6: !moore.unpacked<assoc<i1, string>>,
%arg7: !moore.unpacked<queue<i1>>,
%arg8: !moore.unpacked<queue<i1, 9001>>
Copy link
Member

Choose a reason for hiding this comment

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

Nit: the mnemonic i1 could be a bit confusing since it could be the built-in integer type or the Moore type here if you don't know what the parser does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that is very true. I was thinking of using b1 and l1 to distinguish this from the builtin integers. On the other hand, we might want to eventually replace IntType with a LogicType for l42 and use the builtin integers for i42, so the i1 might be easier to update in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any preferences @maerhart 😁 ?

Copy link
Member

Choose a reason for hiding this comment

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

If the plan is to replace the two-valued Moore IntType with the built-in, I think keeping it as i1 for now makes a lot of sense. 🙂

Comment on lines 71 to 72
auto ui42 = IntType::getInt(&context, 42);
auto ul42 = IntType::getLogic(&context, 42);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Maybe calling it SBVType or similar might be less confusing since there is an int type in SystemVerilog, and this type is not a 1-1 correspondence to that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm yeah. SystemVerilog is very inconsistent. It calls simple bit vectors "integral" data types, or sometimes also "integers" exchangeably. And then there's the weird integer atoms which can be the int or integer you mentioned. My thinking here was that these are actually all integer data types, and the standard is just very clunky and beats around the bush with the simple bit vector types. Inside the dialect and the IR, it would be great to never have to think about the weird stuff that is in the standard, and just think of these values as integers.

Copy link
Member

Choose a reason for hiding this comment

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

I see. In that case, the name is perfectly fine. I wish the SV standard was written less confusingly 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh yeah 😢

@hailongSun2000
Copy link
Member

Let's say I have a !moore.packed<range<i1, 4:0>>. Would that be equivalent to !moore.i5? Both bit and bit [0:0] correspond to !moore.i1, right? But is bit [0:0][1:0] considered a simple bitvector? Is there some canonicalization necessary or does Slang already canonicalize this?

You're right! I agree with the first two points. However, SystemVerilog doesn't consider bit[0:0][1:0] as a simple bit vector.
We can see IEEE 1800-2017 § 6.11.1(quote: The packed structure types (see 7.2) and multidimensional packed array types (see 7.4) are not simple bit vector types, but each is equivalent (see 6.22.2) to some simple bit vector type, to and from which it can be easily converted.).

@fabianschuiki
Copy link
Contributor Author

Let's say I have a !moore.packed<range<i1, 4:0>>. Would that be equivalent to !moore.i5? Both bit and bit [0:0] correspond to !moore.i1, right? But is bit [0:0][1:0] considered a simple bitvector? Is there some canonicalization necessary or does Slang already canonicalize this?

With this PR, ImportVerilog maps things that the standard describes as "simple bit vectors" to the Moore integer data type. int -> i32, integer -> l32, bit [41:0] -> i42, logic [41:0] -> l42. It also canonicalizes bit to i1 and bit [0:0] to i1. This is only done for a single packed dimension though, so bit [3:0][4:0] becomes range<i5, 3:0>, and bit [1:0][0:0] becomes range<i1, 1:0>. It also maps bit vectors that don't strictly start at 0 to the integer data type, so bit [4:3] maps to i2 even though it's not strictly a simple bit vector type. Canonicalizing these range offsets away during the lowering is going to make working with the IR a lot easier.

Copy link
Member

@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.

Thanks for the clarifications, makes sense to me now!

The core type most SystemVerilog expressions are interested in and
operate on is a "simple bit vector type". These are individual integer
atoms like `bit` or `logic`, integral types like `int`, or packed arrays
with a single dimension and an integer atom inner type, like
`bit [42:0]`. So in a nutshell, simple bit vector types are MLIR's `i42`
in the two-valued (`bit`) case, or the four-valued equivalent (`logic`).

Up until this point, the Moore dialect reflected this pattern by
providing and `IntType` for the integer atoms like `bit` and `int`, and
using the `PackedRangeDim` for single dimension vectors of `bit`. A
`SimpleBitVectorType` helper struct would then summarize the _actual_
bit vector that was expressed by the packed range and integer atom. This
makes working with the types in TableGen files very annoying, since the
thing you are actually interested in -- the simple bit vector -- is not
a propery MLIR type, but more like a helper struct on the side.

This commit rips out the existing `IntType` and its composition with a
packed array dimension, and replaces it with a proper simple bit vector
type that is actually an MLIR type. As a result, SystemVerilog types
like `int unsigned`, `bit [42:0]`, `reg`, `logic signed [31:0]`, or
`integer` are all translated into the same MLIR type. This new simple
bit vector MLIR type retains the `IntType` name, and prints as
`!moore.i42` or `!moore.l42`, depending on whether it is a two-valued or
four-valued integer. Single `bit` and `logic` atoms become `i1` and `l1`
respectively.

This makes the Moore type system a lot easier to work with and removes
a lot of unnecessary noise. Operations can now simply use
`llvm::isa<IntType>` to check if a value is a simple bit vector.
@fabianschuiki fabianschuiki merged commit 6a2b628 into main May 9, 2024
4 checks passed
@fabianschuiki fabianschuiki deleted the fschuiki/moore-sbvt branch May 9, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Moore Verilog/SystemVerilog Involving a Verilog dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants