[ImportVerilog] Support for sized unpacked arrays in 'inside' expressions #7971
Conversation
There was a problem hiding this comment.
LGTM 💯 Thanks @ladisgin a lot for this work 😄!
@fabianschuiki WDYT 🤔
| cond = conditions.back(); | ||
| conditions | ||
| .pop_back(); // avoiding repetition of cond in the vector |
There was a problem hiding this comment.
Thanks a lot for working on this! 🙂
This might make the code hard to understand. Could you instead move the conditions.push_back(cond); below into the if branches that need it? That should make the code clearer (no removing and adding back of the condition).
| if (!type) { | ||
| mlir::emitError(loc, "can't convert slang::ast::FixedSizeUnpackedArrayType " | ||
| "to moore::UnpackedArrayType"); | ||
| } |
There was a problem hiding this comment.
Thanks for checking for errors here 😃! You also need to return with some form of error here, ideally by making the function return type a LogicalResult and doing a return failure(); here. The convertType function also already produces a diagnostic explaining to the user what exact type couldn't be converted, so you don't need to generate an additional error here 👍.
There was a problem hiding this comment.
And to add to Fabian's comment, make sure your tests hit all these error message cases.
| } else { | ||
| mlir::emitError(loc, | ||
| "only singular values and fixed-size unpacked arrays " | ||
| "allowed as elements of unpacked arrays in 'inside' " | ||
| "expressions"); | ||
| } |
There was a problem hiding this comment.
Similar to above, a return failure(); would be great here 🙂
| context.collectConditionsForUnpackedArray(uaType, value, | ||
| conditions, lhs, loc); |
There was a problem hiding this comment.
Regarding the LogicalResult return type comment from below, you can check for that here like this:
if (failed(context.collect...))
return {};
| conditions, lhs, loc); | ||
| cond = conditions.back(); | ||
| conditions | ||
| .pop_back(); // avoiding repetition of cond in the vector |
There was a problem hiding this comment.
super nit: comment before the line to keep the code on one line.
| conditions | ||
| .pop_back(); // avoiding repetition of cond in the vector | ||
| } else { | ||
| mlir::emitError(loc, "unsized unpacked arrays in 'inside' " |
There was a problem hiding this comment.
When you do the LogicalResult conversion, you can just return mlir::emitError.
It might be useful to emit the error on the op? I'm not sure, I haven't thought about it.
| const slang::ast::FixedSizeUnpackedArrayType &slangType, | ||
| Value upackedArrayValue, SmallVector<Value> &conditions, Value lhs, | ||
| Location loc) { | ||
| Value cond; |
There was a problem hiding this comment.
This looks like it can be sunk into the condition branch in the loop.
| } else { | ||
| cond = builder.create<moore::EqOp>(loc, lhs, elemValue); | ||
| } | ||
| conditions.push_back(cond); |
There was a problem hiding this comment.
From the comment above, the cond can sink towards here. Or be eliminated by switching to a trinary instead of a if. Or elminated by duplicating the push_back.
| @@ -68,8 +68,8 @@ endmodule | |||
|
|
|||
| // ----- | |||
| module Foo; | |||
| int a, b[3]; | |||
| // expected-error @below {{unpacked arrays in 'inside' expressions not supported}} | |||
| int a, b[]; | |||
There was a problem hiding this comment.
you added a lot more errors. Please add checks for them.
Support for sized unpacked arrays in 'inside' expressions. Implementation recursively traverses the array to find the singular value and compares all found ones with the lhs, adding the condition results to the original conditions vector.