[ImportVerilog] Add basic expressions #6788

fabianschuiki · 2024-03-05T23:26:52Z

Extend the ImportVerilog conversion to support most of the basic expressions that commonly appear in SystemVerilog input files. Also add the correpsonding expression ops to the Moore dialect, and finally get rid of the old MIR expressions file which is now obsolete.

Thanks @hailongSun2000 and @albertethon for doing a lot of the leg work to get expression support in!

maerhart

Wow, this adds a lot of cool stuff! I've only reviewed part of it, will look at the rest later.

include/circt/Dialect/Moore/MooreTypes.td

lib/Dialect/Moore/MooreOps.cpp

maerhart · 2024-03-06T07:14:29Z

lib/Conversion/ImportVerilog/Expressions.cpp

+    mlir::emitError(loc, "expression of type ")
+        << value.getType() << " cannot be cast to a simple bit vector";
+    return {};
+  }
+
+  /// Helper function to convert a value to its "truthy" boolean value.
+  Value convertToBool(Value value) {
+    if (!value)
+      return {};
+    if (auto type = dyn_cast_or_null<moore::IntType>(value.getType()))
+      if (type.getBitSize() == 1)
+        return value;
+    if (auto type = dyn_cast_or_null<moore::UnpackedType>(value.getType()))
+      return builder.create<moore::BoolCastOp>(loc, value);
+    mlir::emitError(loc, "expression of type ")
+        << value.getType() << " cannot be cast to a boolean";


Those two errors don't have a regression test, but not sure if it's easy to add one that only triggers them.

I'm not entirely sure how to trigger these two. Slang's type checker should prevent this from ever appearing in the AST. But I'm also not sure if ImportVerilog covers all cases, and it seemed better to have a defensive error here since this is user-facing.

lib/Conversion/ImportVerilog/Expressions.cpp

maerhart · 2024-03-06T07:30:34Z

lib/Conversion/ImportVerilog/Expressions.cpp

+      auto both = builder.create<moore::AndOp>(loc, lhs, rhs);
+      auto notBoth = builder.create<moore::AndOp>(loc, notLHS, notRHS);
+      return builder.create<moore::OrOp>(loc, both, notBoth);
+    }


Just wondering: what's the rationale of not having implication operations? Of course, there has to be some cut-off on what should be its own operation and which ones would be decomposed to more elementary ops. Comb goes the quite minimalistic route of not even having a not operation (which makes sense for its use-case). Where does/should moore position itself on this spectrum and why?

Just wondering: what's the rationale of not having implication operations? Of course, there has to be some cut-off on what should be its own operation and which ones would be decomposed to more elementary ops. Comb goes the quite minimalistic route of not even having a not operation (which makes sense for its use-case). Where does/should moore position itself on this spectrum and why?

May I ask a question? Why does Comb not have a not operation?

There is a short section about this in the rationale: https://circt.llvm.org/docs/Dialects/Comb/RationaleComb/#no-complement-negate-zext-sext-operators

Thanks so much. I have to get familiar with these rationale.

@maerhart This is a great point. At the moment it's a bit arbitrary, since most of these ops only work on integers. Arguably the reduction operators could also all just be more fundamental operators, but in that case you could argue in favor of them for IR compactness. The implication ops don't have that kind of explosion, they are truly just syntactic sugar.

Some of these operators are overloadable. I'm not entirely sure how they are represented in Slang's AST. For example, if you add two class instances that have the add operator overloaded (is that even a thing in SV?). Does that still show up as a regular "add" expression in the AST, or does Slang already resolve that to the operator overload.

I'd say create as few ops as possible, without causing any IR bloat like the reductions would. Very unprincipled 🤷

Op choices are an art informed by decades of experience from getting it wrong. You want to model at the level of behavior you are interested in transforming. You don't want redundancy. You want compactness. You don't want syntactic sugar. You don't want to have to infer (lift) common cases from a sea of ops. There are lots of conflicting goals.

An easy heuristic is that the number of ops to represent a concept should not scale with the size of the data type involved. Hence reductions are fine, matching a sea of extracts of O(sizeof(type)) is bad. add of constant is good, sequence of increments is bad. xor of -1 is fine for not, since that isn't dependent on sizeof type.

There is a short section about this in the rationale: https://circt.llvm.org/docs/Dialects/Comb/RationaleComb/#no-complement-negate-zext-sext-operators

Thanks again for letting me know the benefit of rationales 😄 .

lib/Conversion/ImportVerilog/Expressions.cpp

maerhart · 2024-03-06T07:53:19Z

include/circt/Dialect/Moore/MooreOps.td

+//===----------------------------------------------------------------------===//
+
+def ConstantOp : MooreOp<"constant", [Pure]> {
+  let summary = "A constant integer value";


What's the plan for supporting constant x and z values? Extend this op to take a new custom attribute that can parse a string with x and z in it, or something else?

Yes, that would be my approach. Define a new attribute that has two APInts internally to capture which of the four values it is. And then possibly funnel all SV constants through that new attribute. Or allow mixing of IntegerAttr and SVIntegerAttr, but that would make folders and canonicalizers more annoying to write.

If you need expertise on the ways it shouldn't be done, I'm your man. 😉

@fabianschuiki - I have an implementation that I've recently finished to handle Z and X and it does indeed rely on APInt as well as the slang SVInt class. I'm happy to take a stab at that when my time permits.

I hadn't considered using SVInt. That might actually be a really good approach. Just put that into attribute storage and intern it in the context.

Ah but that would expose Slang types in CIRCT, outside the confines of ImportVerilog. One of the design goals is to to have all Slang types and specifics hidden away in that library, and not have it leak out into other parts of CIRCT. So Moore will want its own version of this. Could be just two APInts put together, with all the interesting operations defined on them. And then an attribute that wraps that into an interned SVIntegerAttr.

At this point I feel I cannot avoid pointing out my misadventures of last summer: APLogic and APLogicConstant . APLogic is/was effectively a nine-valued variant of APInt using 4 bits per digit. APLogicConstant was intended as an immutable container that picked its encoding depending on the stored value, similar to slang's SVInt. In the two-valued case it was, regarding its structure, an immutable APInt with the encoding information tucked away in the padding.
I ended up shelving them. The switching between 1 Bit, 2 Bit and 4 Bit encoding made the implementation of the operations far too complex for my liking. And I felt general arithmetic on arbitrary strings of nine-valued logic is too irrelevant in practice to be worth adding the complexity for.
I'd be happy if something useful could rise out of its smoking ruins. But on the other hand, wouldn't it be possible to just ask if we can re-license SVInt and simply copy it?

For my Many Valued Logic Dialect shenanigans I have since switched to a completely different approach, that does not even try to be useful for storing integers. I have implemented some basic folds with that one, but they are not on github at the moment. Since it is also made for nine valued logic and with parameterized width types in mind, it probably is not the right choice for Moore.

I've been meaning to ask about the old PR every time I do a round of reviewing. Should we close it?

Some possibilities:

different attribute kinds for simple v.s. complex contants (doesn't have to affect parsing/printing).

different constant ops for the different cases (make the simple case simple, esp since lots of code probably at first will just not touch complex constants).

I personally still lean toward different types, the only hesitation is that parameterized are different enough that maybe any non-simple type should just go through whatever handles that.

We could try to build this out here in the Moore dialect and for Moore folds, where every constant would be complex (to prevent folders/canonicalizers from dealing with two overlapping attribute types). And then see how we can pull out the 4-valued and 9-valued flavors into the HW dialect as a core CIRCT type.

hailongSun2000 · 2024-03-11T05:30:05Z

Extend the ImportVerilog conversion to support most of the basic expressions that commonly appear in SystemVerilog input files. Also add the correpsonding expression ops to the Moore dialect, and finally get rid of the old MIR expressions file which is now obsolete.

Thanks @hailongSun2000 and @albertethon for doing a lot of the leg work to get expression support in!

Thanks for @fabianschuiki's patient guidance and code review. It's an honor for me to work with you. You are my idol--a gentle and erudite man--who has lots of things waiting for me to learn 🙇 .

mikeurbach

I know there was an interesting discussion about kinds of logic in the ODM today, but since you asked for review on this PR, I have looked through it and the various expressions look good to me.

fabianschuiki · 2024-03-13T20:17:15Z

I know there was an interesting discussion about kinds of logic in the ODM today

Yeah, it sounds like once all the expressions are in we'll have a better foundation to go tighten up the type system and introduce a dedicated SimpleBitVectorType that all the expressions can work on. After that it might be time to phase out some of the other types that end up just being syntactic sugar.

fabianschuiki · 2024-03-13T21:14:16Z

Any objections to me landing this?

@hailongSun2000

Extend the `ImportVerilog` conversion to support most of the basic expressions that commonly appear in SystemVerilog input files. Also add the correpsonding expression ops to the Moore dialect, and finally get rid of the old MIR expressions file which is now obsolete. Thanks @hailongSun2000 and @albertethon for doing a lot of the leg work to get expression support in! Co-authored-by: Hailong Sun <hailong.sun@terapines.com> Co-authored-by: ShiZuoye <albertethon@163.com>

mikeurbach · 2024-03-13T21:41:37Z

Not from me, I looked through the implementation and it looks nice. Not sure I'm the best one to speak to high-level ideas here, but I'm in favor of making forward progress.

maerhart

Sorry for taking so long to complete the review!
The operation definitions look nice and clean with good descriptions.
I've not compared them to the standard, so I just trust you on that 😄

include/circt/Dialect/Moore/MooreOps.td

Co-authored-by: Martin Erhart <maerhart@outlook.com>

fabianschuiki · 2024-03-13T23:58:08Z

Thanks 🥳!

fabianschuiki added the Verilog/SystemVerilog Involving a Verilog dialect label Mar 5, 2024

fabianschuiki requested review from uenoku, maerhart, dtzSiFive and hailongSun2000 March 5, 2024 23:26

maerhart reviewed Mar 6, 2024

View reviewed changes

fabianschuiki force-pushed the fschuiki/slang-expressions branch from 316db7a to 0095b52 Compare March 6, 2024 21:19

itaras20 mentioned this pull request Mar 7, 2024

[ImportVerilog] Support for String Types, String Literals and Real Literals fabianschuiki/circt#41

Open

fabianschuiki force-pushed the fschuiki/slang-expressions branch from 0095b52 to 9f9ab96 Compare March 8, 2024 19:22

mikeurbach reviewed Mar 13, 2024

View reviewed changes

fabianschuiki force-pushed the fschuiki/slang-expressions branch from 9f9ab96 to 2198a8e Compare March 13, 2024 21:15

maerhart approved these changes Mar 13, 2024

View reviewed changes

include/circt/Dialect/Moore/MooreOps.td Outdated Show resolved Hide resolved

Update include/circt/Dialect/Moore/MooreOps.td

021b1be

Co-authored-by: Martin Erhart <maerhart@outlook.com>

fabianschuiki merged commit c970974 into main Mar 13, 2024

fabianschuiki deleted the fschuiki/slang-expressions branch March 13, 2024 23:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ImportVerilog] Add basic expressions #6788

[ImportVerilog] Add basic expressions #6788

fabianschuiki commented Mar 5, 2024

maerhart left a comment

maerhart Mar 6, 2024

fabianschuiki Mar 6, 2024

maerhart Mar 6, 2024

hailongSun2000 Mar 6, 2024

maerhart Mar 6, 2024

hailongSun2000 Mar 6, 2024

fabianschuiki Mar 6, 2024

darthscsi Mar 11, 2024

hailongSun2000 Mar 15, 2024

maerhart Mar 6, 2024

fabianschuiki Mar 6, 2024

fzi-hielscher Mar 6, 2024

itaras20 Mar 7, 2024

fabianschuiki Mar 7, 2024

fabianschuiki Mar 8, 2024

fzi-hielscher Mar 8, 2024

darthscsi Mar 11, 2024

darthscsi Mar 11, 2024

fabianschuiki Mar 12, 2024

hailongSun2000 commented Mar 11, 2024

mikeurbach left a comment

fabianschuiki commented Mar 13, 2024

fabianschuiki commented Mar 13, 2024

mikeurbach commented Mar 13, 2024

maerhart left a comment

fabianschuiki commented Mar 13, 2024

[ImportVerilog] Add basic expressions #6788

[ImportVerilog] Add basic expressions #6788

Conversation

fabianschuiki commented Mar 5, 2024

maerhart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hailongSun2000 commented Mar 11, 2024

mikeurbach left a comment

Choose a reason for hiding this comment

fabianschuiki commented Mar 13, 2024

fabianschuiki commented Mar 13, 2024

mikeurbach commented Mar 13, 2024

maerhart left a comment

Choose a reason for hiding this comment

fabianschuiki commented Mar 13, 2024