[ImportVerilog] Support case inside statements#10090
[ImportVerilog] Support case inside statements#10090sunhailong2001 wants to merge 1 commit intollvm:mainfrom
Conversation
|
Hey, @fabianschuiki ! Thanks for reviewing this. 😃 |
| Value visit(const slang::ast::ValueRangeExpression &expr) { | ||
| assert(!context.lvalueStack.empty() && "inside pushes lvalue"); | ||
| auto lhs = context.lvalueStack.back(); | ||
| // Handle ranges. | ||
| auto lowBound = context.convertToSimpleBitVector( | ||
| context.convertRvalueExpression(expr.left())); | ||
| auto highBound = context.convertToSimpleBitVector( | ||
| context.convertRvalueExpression(expr.right())); | ||
| if (!lowBound || !highBound) | ||
| return {}; | ||
| Value leftValue, rightValue; | ||
| // Determine if the expression on the left-hand side is inclusively | ||
| // within the range. | ||
| if (expr.left().type->isSigned() || expr.left().type->isSigned()) { | ||
| leftValue = moore::SgeOp::create(builder, loc, lhs, lowBound); | ||
| } else { | ||
| leftValue = moore::UgeOp::create(builder, loc, lhs, lowBound); | ||
| } | ||
| if (expr.right().type->isSigned() || expr.left().type->isSigned()) { | ||
| rightValue = moore::SleOp::create(builder, loc, lhs, highBound); | ||
| } else { | ||
| rightValue = moore::UleOp::create(builder, loc, lhs, highBound); | ||
| } | ||
| return moore::AndOp::create(builder, loc, leftValue, rightValue); | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm wondering if using lvalueStack for this is too brittle. We have used the lvalue stack to specifically handle Slang's AST nodes that refer the LHS in assignments like a += b. Checking whether one expression is inside the range described by a ValueRangeExpression feels like it doesn't involve lvalues at all.
My suggestion: let's add a convertInsideValueRangeCheck(Value value, const slang::ast::ValueRangeExpression &range), and move the content of this visit(...) function in there. Then you don't need to use lvalueStack; instead, you can directly use the value passed to the function. The visit(InsideExpression) and visit(CaseStatement) functions can then call the new convertInsideValueRangeCheck directly to implement their inside checks 😃
What do you think?
There was a problem hiding this comment.
Done, but it lacks elegance. Moreover, if convertInsideValueRangeCheck processes the entire range list or case item, for the non-ValueRange cases, an extra function invocation is required. 😞
There was a problem hiding this comment.
Hmmm do you think the code at the moment doesn't support certain inputs? Were you thinking of x inside {[a:b], 42, [c:d]}? If case items and inside expressions can have these lists of expressions, maybe we could move more things into convertInsideValueRangeCheck and rename it to something like convertInsideCheck, and pass the entire list of expressions to that function? That would allow us to share almost all code between case items and inside... What do you think?
sunhailong2001
force-pushed
the
hailong/case-inside
branch
from
3532795 to
a08f66b
Compare
Support translating 'case (...) inside' statements. And add the `convertInsideValueRangeCheck` function to handle the `slang::ast::ValueRangeExpression` dedicatedly.
sunhailong2001
force-pushed
the
hailong/case-inside
branch
from
a08f66b to
c9852c0
Compare
2 participants
Support translating
case (...) insidestatements. As part of this change, refactor existingslang::ast::InsideExpressionhandling by movingValueRangeExpressiontranslation into a dedicated visit.