-
Notifications
You must be signed in to change notification settings - Fork 404
[Moore][ImportVerilog] Add real-valued ops + real-aware import; refactor logical handling #9122
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
Conversation
097315a to
d56317d
Compare
1f1bf2c to
50c009e
Compare
fb132a4 to
658622c
Compare
| Type lhsTy = context.convertType(*expr.left().type, loc); | ||
| Type rhsTy = context.convertType(*expr.right().type, loc); | ||
|
|
||
| if (!lhsTy || !rhsTy) | ||
| return {}; | ||
|
|
||
| assert(lhsTy == rhsTy && | ||
| "Slang should have converted both operands to the same type!"); | ||
| Type targetTy = lhsTy; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use the type of the binary expression directly: expr.type. Slang computes that using the rules for operands outlined in the SV spec, including finding the widest operand width, signness matching, etc. I think it does the same thing for reals, where a shortreal + real would be determined to have type real for the + result.
That also means you don't need to assert equality on the LHS and RHS type. The assert will abort the compiler. If you don't assert, the conversions further down fail with a proper diagnostic pointing at the failing input, which is going to be much easier to debug and track down if it happens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, makes sense. Wasn't aware of expr.type but that simplifies this a lot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit too much as it turns out! expr.type holds the return type, which sometimes but not always is equal to the RHS types.
| // First check whether we need real or integral BOps | ||
| const auto *rhsFloatType = | ||
| expr.right().type->as_if<slang::ast::FloatingType>(); | ||
| const auto *lhsFloatType = | ||
| expr.left().type->as_if<slang::ast::FloatingType>(); | ||
|
|
||
| // If either op is real-typed, treat as real BOp. Bail here. | ||
| if (rhsFloatType || lhsFloatType) | ||
| return visitRealBOp(expr); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: I think you can just check expr.type and see if it is a float. If it is, call out to visitRealBOp.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! Fixed it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least in here, we can't make the shortcut: a BinaryOperator::Equality will have return type i1, so this currently fails. This might also be a bug for the "normal" sbv pipeline if operands are cast to i1!
fc55461 to
15433de
Compare
fabianschuiki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really really cool! Thanks a lot for adding all of this 🥳 💯!
15433de to
93e6af8
Compare
Thanks for the thorough review! Let's hope this fixes some more sv-tests 😄 |
4d11bb2 to
39abea5
Compare
…tor logical handling Introduce full **real (f32/f64)** operator support in the Moore dialect and the ImportVerilog conversion, including unary, arithmetic, equality, relational, logical, and ++/-- forms. Refactor logical operator lowering to a reusable helper and add comprehensive tests. - **Dialect (`MooreOps.td`):** - New unary real op: `NegRealOp` -> `moore.fneg`. - Real binary op base: `BinaryRealOpBase` for typed f32/f64 operands/results. - Arithmetic: `AddRealOp` (`fadd`), `SubRealOp` (`fsub`), `MulRealOp` (`fmul`), `DivRealOp` (`fdiv`), `PowRealOp` (`fpow`). - Equality: `EqRealOp` (`feq`), `NeRealOp` (`fne`). - Relational: `FltOp` (`flt`), `FleOp` (`fle`), `FgtOp` (`fgt`), `FgeOp` (`fge`). - All ops follow IEEE 1800-2023 §11.3.1, §11.4.3–§11.4.5 semantics. - **Import (`Expressions.cpp`):** - Add `visitRealUOp` and `visitRealBOp` to lower real-typed unary/binary expressions. - Implement pre/post **increment/decrement** for real variables via `fadd`/`fsub` with an exact `APFloat` **"1.0"** (IEEEsingle/IEEEdouble) constructor. - Add `moore.fneg` for unary minus; unary plus becomes a no-op. - Map real comparisons to `feq/fne/flt/fle/fgt/fge`. - **Refactor logical ops** into `buildLogicalBOp(...)` (optional `Domain` param), shared by int/real paths. (Short-circuit remains TODO.) - Route mixed/real expressions early to the real path; keep existing integral flow otherwise. - **Tests (`basic.sv`):** - New end-to-end checks for f64/f32 arithmetic, comparisons, logicals, unary ops, and ++/--. - Assert exact opcode emission (`moore.fadd/fsub/fmul/fdiv/fpow/feq/fne/flt/fle/fgt/fge/fneg`) and constant forms for `1.0`.
39abea5 to
0881ad1
Compare
Introduce full real (f32/f64) operator support in the Moore dialect and the ImportVerilog conversion, including unary, arithmetic, equality, relational, logical, and ++/-- forms. Refactor logical operator lowering to a reusable helper and add comprehensive tests.
Dialect (
MooreOps.td):NegRealOp->moore.fneg.BinaryRealOpBasefor typed f32/f64 operands/results.AddRealOp(fadd),SubRealOp(fsub),MulRealOp(fmul),DivRealOp(fdiv),PowRealOp(fpow).EqRealOp(feq),NeRealOp(fne).FltOp(flt),FleOp(fle),FgtOp(fgt),FgeOp(fge).Import (
Expressions.cpp):visitRealUOpandvisitRealBOpto lower real-typed unary/binary expressions.fadd/fsubwith an exactAPFloat"1.0" (IEEEsingle/IEEEdouble) constructor.moore.fnegfor unary minus; unary plus becomes a no-op.feq/fne/flt/fle/fgt/fge.buildLogicalBOp(...)(optionalDomainparam), shared by int/real paths. (Short-circuit remains TODO.)Tests (
basic.sv):moore.fadd/fsub/fmul/fdiv/fpow/feq/fne/flt/fle/fgt/fge/fneg) and constant forms for1.0.