[ImportVerilog] Add conditional operator. #6950

angelzzzzz · 2024-04-25T05:59:11Z

Handle conditional operator ?:.
Use scf.if to control the selection of expressions, and return the selected value through scf.yield.

hailongSun2000

LGTM!

maerhart · 2024-04-25T06:20:51Z

The lowering does not match the standard for 4-valued conditions with X or Z. Apparently it should then evaluate both the 2nd and 3rd operand, compare them for equivalence, etc.
The tests only seem to consider 2-valued conditions. Should we display an error for 4-valued conditions for now?

angelzzzzz · 2024-04-26T07:15:11Z

The lowering does not match the standard for 4-valued conditions with X or Z. Apparently it should then evaluate both the 2nd and 3rd operand, compare them for equivalence, etc. The tests only seem to consider 2-valued conditions. Should we display an error for 4-valued conditions for now?

Good point! I have added some error tests for 4-valued conditions and look forward to your suggestions.

maerhart · 2024-04-26T08:10:25Z

Good point! I have added some error tests for 4-valued conditions and look forward to your suggestions.

Thanks! This only concerns literals now, right? What if the condition expression is not a literal but, e.g., some input port that is 4-valued and may contain 'x'? Can't we check the type of the expression for 4-valuedness instead of the expression itself, or would that be too restrictive for our use-cases?

angelzzzzz · 2024-04-28T03:44:45Z

Thanks! This only concerns literals now, right? What if the condition expression is not a literal but, e.g., some input port that is 4-valued and may contain 'x'? Can't we check the type of the expression for 4-valuedness instead of the expression itself, or would that be too restrictive for our use-cases?

Thanks for your thoughtful consideration! Do you mean we need to check the runtime values? If so, I guess we can only analyze the type of expression at compile time rather than at runtime. Additionally, we cannot determine whether it is x or z based on the type of expression, e.g., x and z may be logic or reg type. And we can only get the kind of x and z -- UnbasedUnsizedIntegerLiteral.

fabianschuiki · 2024-04-30T18:09:11Z

I think what @maerhart is getting at is the complicated handling that is necessary when the condition is X or Z ("11.4.11 Conditional operator" in IEEE 1800-2017). When the condition is of a two-valued type (derived from bit), your scf.if solution is perfect. You either evaluate the left or right expression, depending on whether the condition is 1 or 0. If the condition is of a four-valued type however (derived from logic), and it is X or Z, you have to evaluate both the left and right expression, and merge them. The merging is very annoying -- there's an entire list of rules mentioned in 11.4.11. It goes something like the following:

If the condition is 1, evaluate and return the left expression. If the condition is 0, evaluate and return the right expression. Otherwise:
Evaluate the left and right expression. Check them for equality (== or ===, not sure which). If they are equal, return one of them. Otherwise:
If the left and right expression are of integral data type (can be cast to a simple bit vector), apply a special bitwise truth table mentioned in 11.4.11 between the left and right expression. Otherwise:
Follow the remaining rules in 11.4.11.
If no rule was able to handle the ambiguity, return the default value for the type of the condition (0 for bit and X for logic).

So I think one thing you could do is explicitly check if the condition is 0 or 1, and otherwise print an error, raise an exception, or do something else. For example, pseudocode:

scf.if (cond === 0) {
  %0 = eval leftExpr
  scf.yield %0
} else {
  scf.if (cond === 1) {
    %1 = eval rightExpr
    scf.yield %1
  } else {
    verif.assert %false
  }
}

That would cause the simulation to fail properly if we encounter an unsupported condition in the mux, and we know where to start once we improve the lowering in the future. We'll probably want to have a dedicated op in the Moore dialect that performs the mux/conditional merging of the left and right expression. And we'll want to eventually implement all the rules in 11.4.11 and apply the correct one depending on the data type of what we're muxing.

You can also kick the can down the road and introduce a new op that is similar to scf.if but encodes the special behaviour of the expression:

moore.conditional %cond {
  moore.yield leftExpr
} {
  moore.yield rightExpr
}

That would allow you to have a later lowering pass that takes moore.conditional and replaces it with the correct behaviour. And encapsulating the left and right expression in a region captures the fact that you may only be evaluating one of the expressions depending on the situation.

angelzzzzz · 2024-05-29T09:31:20Z

@fabianschuiki Thank you very much for your suggestion 😃 ! I chose the second method and defined two new operations for the conditional operator: moore.conditional and moore.yield.

hailongSun2000 · 2024-05-29T09:25:23Z

include/circt/Dialect/Moore/MooreOps.td

+    If cond_predicate evaluates to an ambiguous value (x or z), then both the
+    first expression and the second expression shall be evaluated, and compared
+    for logical equivalence. If that comparison is true (1), the operator shall
+    return either the first or second expression. Otherwise the operator returns
+    a result based on the data types of the expressions.


Can I consider logical equivalence as <->?

hailongSun2000 · 2024-05-29T09:38:16Z

lib/Conversion/ImportVerilog/Expressions.cpp

+    else
+      builder.create<moore::YieldOp>(loc, convertToSimpleBitVector(trueValue));
+


convertToSimpleBitVector() may return {} if the type of trueValue isn't SBVT(Like logic [0:5][2:0] p3).

Good point! I'll fix that.

hailongSun2000 · 2024-05-29T09:38:45Z

lib/Conversion/ImportVerilog/Expressions.cpp

+    else
+      builder.create<moore::YieldOp>(loc, convertToSimpleBitVector(falseValue));
+


hailongSun2000 · 2024-05-29T09:46:01Z

include/circt/Dialect/Moore/MooreOps.td

+    If "moore.yield" has any operands, the operands must match the parent
+    operation's results.


How do you ensure the types of operands of moore.yield match the result types of moore.conditional? Maybe you should add the related method to check this.

Nice advice! However, Slang already ensures a match between result type and expressions type, so additional checks seem unnecessary.

I think you still need a check to make sure that the IR is not malformed. Most of the time the IR will come from import through Slang, but in many other cases the input IR will be handwritten in case of unit tests, or it will be generated from transformation passes. It is important that operations check that the IR is correct to catch bugs in those other cases as soon as possible.

The hw::OutputOp, arc::OutputOp, scf::YieldOp, or func::ReturnOp have to do a similar kind of check -- you might find some inspiration on how to do it there. I'm not 100% if the output/yield op verifies that it matches the parent condition/module/arc/func/if op, or if the parent op checks that it has the right terminator.

hailongSun2000 · 2024-05-29T09:49:54Z

test/Conversion/ImportVerilog/basic.sv

+    //===------------------------------------------------------------------===//
+    // Conditional operator
+


nit: It would be better if you could add some non-SBVT tests.

fabianschuiki · 2024-05-30T04:54:35Z

lib/Conversion/ImportVerilog/Expressions.cpp

+    auto trueValue = context.convertExpression(expr.left());
+    if (!trueValue)


In this case you should just return {}, since convertExpression returning a null value indicates that an error has happened that has already been reported in a diagnostic by the expression conversion code. Similar to how you further above check if (!cond) return {};.

If just return {}, it will return an unexpected error that "error: 'moore.conditional' op expects a non-empty block". How can I avoid this error?

That is very strange! This means that even though you returned {} here, the Slang-to-MLIR conversion continued and at the end the MLIR verifier was called to make sure that the IR is correct. We might be missing additional checks for if (!<expr>) return {}; elsewhere in the converter.

In theory what should happen is: when you encounter an error and the convert* functions start returning {}, this abort and return with {} should propagate all the way to the top, and in ImportVerilog, conversion should be aborted without the MLIR verifier running at all.

Do you have the input Verilog that causes this issue? I can try to help you find why this happens 😃

Sure! Here is the input:

module top(); int a,b; initial begin a = b ? 'x : b; end endmodule

Great catch! It looks like there was a very subtle bug in convertStatement where a failure from convertExpression would not be propagated properly. That was also the reason why we were able to check for multiple errors in errors.sv in parallel, which was suspicious. (Import should have aborted after first error.)

I have pushed a fix straight to the main branch: dd68e3c

Nice! Fixing this bug has been of great help to me. 🥰

fabianschuiki · 2024-05-30T04:54:48Z

lib/Conversion/ImportVerilog/Expressions.cpp

+    auto falseValue = context.convertExpression(expr.right());
+    if (!falseValue)


Same as above: return {};

angelzzzzz · 2024-06-03T06:12:06Z

@fabianschuiki @hailongSun2000 I sincerely appreciate your suggestions! I have added verify to YieldOp, and fixed some issues. I'm glad to hear more suggestions. 😄

fabianschuiki · 2024-06-03T17:55:49Z

This looks very cool! Thanks for adding all the fixes and the new ops 😍.

Can you add a test to test/Dialect/Moore/basic.mlir for moore.conditional and moore.yield? That file tests whether the ops in the dialect parse and print properly. So basically you'd add an example of your new ops there and then use CHECK: to ensure that the ops get printed out in the exact same way 🙂

Similarly, since you've added new error diagnostics to the dialect in moore.yield, can you add checks for each of the diagnostics to test/Dialect/Moore/errors.mlir?

angelzzzzz · 2024-06-04T06:23:40Z

This looks very cool! Thanks for adding all the fixes and the new ops 😍.

Can you add a test to test/Dialect/Moore/basic.mlir for moore.conditional and moore.yield? That file tests whether the ops in the dialect parse and print properly. So basically you'd add an example of your new ops there and then use CHECK: to ensure that the ops get printed out in the exact same way 🙂

Similarly, since you've added new error diagnostics to the dialect in moore.yield, can you add checks for each of the diagnostics to test/Dialect/Moore/errors.mlir?

That's a good idea! I have added several tests for moore.conditional and moore.yield and I will add more if we need more tests. 😄

fabianschuiki

Cool, looks really nice! Thanks a lot for all your work 🥳

hailongSun2000 requested review from fabianschuiki, uenoku, maerhart and hailongSun2000 April 25, 2024 06:00

hailongSun2000 added the Verilog/SystemVerilog Involving a Verilog dialect label Apr 25, 2024

hailongSun2000 approved these changes Apr 25, 2024

View reviewed changes

angelzzzzz force-pushed the slang-conditional branch from 7bbb1bb to 2bb3636 Compare April 26, 2024 07:09

angelzzzzz force-pushed the slang-conditional branch from 2bb3636 to d374251 Compare April 26, 2024 07:18

angelzzzzz force-pushed the slang-conditional branch from d374251 to 1b030b9 Compare May 29, 2024 09:15

angelzzzzz requested a review from hailongSun2000 May 29, 2024 09:16

hailongSun2000 reviewed May 29, 2024

View reviewed changes

angelzzzzz force-pushed the slang-conditional branch from 1b030b9 to ae9e495 Compare May 29, 2024 10:13

fabianschuiki reviewed May 30, 2024

View reviewed changes

angelzzzzz force-pushed the slang-conditional branch from ae9e495 to 77f8f37 Compare June 3, 2024 05:41

angelzzzzz requested review from hailongSun2000 and fabianschuiki June 3, 2024 05:59

angelzzzzz force-pushed the slang-conditional branch from 77f8f37 to 065d35d Compare June 4, 2024 05:54

fabianschuiki approved these changes Jun 4, 2024

View reviewed changes

angelzzzzz force-pushed the slang-conditional branch from 065d35d to f70c5b8 Compare June 5, 2024 01:46

[ImportVerilog] Add conditional operator.

1b4d59f

angelzzzzz force-pushed the slang-conditional branch from f70c5b8 to 1b4d59f Compare June 5, 2024 02:13

hailongSun2000 merged commit c76eb12 into llvm:main Jun 5, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ImportVerilog] Add conditional operator. #6950

[ImportVerilog] Add conditional operator. #6950

angelzzzzz commented Apr 25, 2024

hailongSun2000 left a comment

maerhart commented Apr 25, 2024

angelzzzzz commented Apr 26, 2024

maerhart commented Apr 26, 2024

angelzzzzz commented Apr 28, 2024 •

edited

Loading

fabianschuiki commented Apr 30, 2024

angelzzzzz commented May 29, 2024

hailongSun2000 May 29, 2024

hailongSun2000 May 29, 2024

angelzzzzz May 29, 2024

hailongSun2000 May 29, 2024

hailongSun2000 May 29, 2024

angelzzzzz May 29, 2024

fabianschuiki May 30, 2024

hailongSun2000 May 29, 2024

fabianschuiki May 30, 2024

angelzzzzz May 31, 2024 •

edited

Loading

fabianschuiki May 31, 2024

angelzzzzz May 31, 2024

fabianschuiki May 31, 2024

angelzzzzz Jun 2, 2024

fabianschuiki May 30, 2024

angelzzzzz commented Jun 3, 2024

fabianschuiki commented Jun 3, 2024

angelzzzzz commented Jun 4, 2024

fabianschuiki left a comment •

edited

Loading

		else
		builder.create<moore::YieldOp>(loc, convertToSimpleBitVector(trueValue));

		If "moore.yield" has any operands, the operands must match the parent
		operation's results.

		//===------------------------------------------------------------------===//
		// Conditional operator

		auto trueValue = context.convertExpression(expr.left());
		if (!trueValue)

		auto falseValue = context.convertExpression(expr.right());
		if (!falseValue)

[ImportVerilog] Add conditional operator. #6950

[ImportVerilog] Add conditional operator. #6950

Conversation

angelzzzzz commented Apr 25, 2024

hailongSun2000 left a comment

Choose a reason for hiding this comment

maerhart commented Apr 25, 2024

angelzzzzz commented Apr 26, 2024

maerhart commented Apr 26, 2024

angelzzzzz commented Apr 28, 2024 • edited Loading

fabianschuiki commented Apr 30, 2024

angelzzzzz commented May 29, 2024

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

angelzzzzz May 31, 2024 • edited Loading

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

angelzzzzz commented Jun 3, 2024

fabianschuiki commented Jun 3, 2024

angelzzzzz commented Jun 4, 2024

fabianschuiki left a comment • edited Loading

Choose a reason for hiding this comment

angelzzzzz commented Apr 28, 2024 •

edited

Loading

angelzzzzz May 31, 2024 •

edited

Loading

fabianschuiki left a comment •

edited

Loading