Skip to content
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.

Sign up for GitHub

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

[ImportVerilog] Support for sized unpacked arrays in 'inside' expressions #7971

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+103 −11
Open

[ImportVerilog] Support for sized unpacked arrays in 'inside' expressions #7971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 61 additions & 9 deletions lib/Conversion/ImportVerilog/Expressions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,19 +554,35 @@ struct RvalueExprVisitor {
// Handle expressions.
if (!listExpr->type->isSimpleBitVector()) {
if (listExpr->type->isUnpackedArray()) {
if (listExpr->type->isFixedSize()) {
const auto &uaType =
listExpr->type->as<slang::ast::FixedSizeUnpackedArrayType>();
auto value = context.convertRvalueExpression(*listExpr);
if (!value)
return {};
context.collectConditionsForUnpackedArray(uaType, value,
conditions, lhs, loc);
Comment on lines +563 to +564
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regarding the LogicalResult return type comment from below, you can check for that here like this:

if (failed(context.collect...))
  return {};

cond = conditions.back();
conditions
.pop_back(); // avoiding repetition of cond in the vector
Comment on lines +565 to +567
Copy link
Contributor

@fabianschuiki fabianschuiki Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super nit: comment before the line to keep the code on one line.

} else {
mlir::emitError(loc, "unsized unpacked arrays in 'inside' "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"expressions not supported");
return {};
}
} else {
mlir::emitError(
loc, "unpacked arrays in 'inside' expressions not supported");
loc,
"only simple bit vectors supported in 'inside' expressions");
return {};
}
mlir::emitError(
loc, "only simple bit vectors supported in 'inside' expressions");
return {};
} else {
auto value = context.convertToSimpleBitVector(
context.convertRvalueExpression(*listExpr));
if (!value)
return {};
cond = builder.create<moore::WildcardEqOp>(loc, lhs, value);
}
auto value = context.convertToSimpleBitVector(
context.convertRvalueExpression(*listExpr));
if (!value)
return {};
cond = builder.create<moore::WildcardEqOp>(loc, lhs, value);
}
conditions.push_back(cond);
}
Expand Down Expand Up @@ -1101,3 +1117,39 @@ Value Context::materializeConversion(Type type, Value value, bool isSigned,
value = builder.create<moore::ConversionOp>(loc, type, value);
return value;
}

void Context::collectConditionsForUnpackedArray(
const slang::ast::FixedSizeUnpackedArrayType &slangType,
Value upackedArrayValue, SmallVector<Value> &conditions, Value lhs,
Location loc) {
Value cond;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like it can be sunk into the condition branch in the loop.

auto type = convertType(slangType);
if (!type) {
mlir::emitError(loc, "can't convert slang::ast::FixedSizeUnpackedArrayType "
"to moore::UnpackedArrayType");
}
Comment on lines +1127 to +1130
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And to add to Fabian's comment, make sure your tests hit all these error message cases.

auto mooreType = dyn_cast<moore::UnpackedArrayType>(type);
const auto &elementType = slangType.elementType;
for (slang::int32_t i = slangType.getFixedRange().lower();
i <= slangType.getFixedRange().upper(); i++) {
auto elemValue = builder.create<moore::ExtractOp>(
loc, mooreType.getElementType(), upackedArrayValue, i);
if (elementType.isUnpackedArray()) {
collectConditionsForUnpackedArray(
elementType.as<slang::ast::FixedSizeUnpackedArrayType>(), elemValue,
conditions, lhs, loc);
} else if (elementType.isSingular()) {
if (elementType.isIntegral()) {
cond = builder.create<moore::WildcardEqOp>(loc, lhs, elemValue);
} else {
cond = builder.create<moore::EqOp>(loc, lhs, elemValue);
}
conditions.push_back(cond);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
mlir::emitError(loc,
"only singular values and fixed-size unpacked arrays "
"allowed as elements of unpacked arrays in 'inside' "
"expressions");
}
Comment on lines +1148 to +1153
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above, a return failure(); would be great here 🙂

}
}
7 changes: 7 additions & 0 deletions lib/Conversion/ImportVerilog/ImportVerilogInternals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,13 @@ struct Context {
Value materializeConstant(const slang::ConstantValue &constant,
const slang::ast::Type &type, Location loc);

/// Helper function to construct conditions for a fixed-size unpacked array in
/// an `inside` expression.
void collectConditionsForUnpackedArray(
const slang::ast::FixedSizeUnpackedArrayType &slangType,
Value upackedArrayValue, SmallVector<Value> &conditions, Value lhs,
Location loc);

/// Convert a list of string literal arguments with formatting specifiers and
/// arguments to be interpolated into a `!moore.format_string` value. Returns
/// failure if an error occurs. Returns a null value if the formatted string
Expand Down
33 changes: 33 additions & 0 deletions test/Conversion/ImportVerilog/basic.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -657,6 +657,10 @@ module Expressions;
bit [31:0] uarrayInt [2];
// CHECK: %m = moore.variable : <l4>
logic [3:0] m;
// CHECK: %ua = moore.variable : <uarray<3 x i32>>
int ua[3];
// CHECK: %ua2d = moore.variable : <uarray<2 x uarray<2 x i32>>>
int ua2d[2][2];

initial begin
// CHECK: moore.constant 0 : i32
Expand Down Expand Up @@ -1076,6 +1080,35 @@ module Expressions;
// CHECK: moore.or [[TMP3]], [[TMP11]] : i1
c = a inside { a, b, [a:b] };

// CHECK: [[TMP1:%.+]] = moore.read %a : <i32>
// CHECK: [[TMP2:%.+]] = moore.read %ua : <uarray<3 x i32>>
// CHECK: [[TMP3:%.+]] = moore.extract [[TMP2]] from 0 : uarray<3 x i32> -> i32
// CHECK: [[TMP4:%.+]] = moore.wildcard_eq [[TMP1]], [[TMP3]] : i32 -> i1
// CHECK: [[TMP5:%.+]] = moore.extract [[TMP2]] from 1 : uarray<3 x i32> -> i32
// CHECK: [[TMP6:%.+]] = moore.wildcard_eq [[TMP1]], [[TMP5]] : i32 -> i1
// CHECK: [[TMP7:%.+]] = moore.extract [[TMP2]] from 2 : uarray<3 x i32> -> i32
// CHECK: [[TMP8:%.+]] = moore.wildcard_eq [[TMP1]], [[TMP7]] : i32 -> i1
// CHECK: [[TMP9:%.+]] = moore.or [[TMP6]], [[TMP8]] : i1
// CHECK: [[TMP10:%.+]] = moore.or [[TMP4]], [[TMP9]] : i1
c = a inside { ua };

// CHECK: [[TMP1:%.+]] = moore.read %a : <i32>
// CHECK: [[TMP2:%.+]] = moore.read %ua2d : <uarray<2 x uarray<2 x i32>>>
// CHECK: [[TMP3:%.+]] = moore.extract [[TMP2]] from 0 : uarray<2 x uarray<2 x i32>> -> uarray<2 x i32>
// CHECK: [[TMP4:%.+]] = moore.extract [[TMP3]] from 0 : uarray<2 x i32> -> i32
// CHECK: [[TMP5:%.+]] = moore.wildcard_eq [[TMP1]], [[TMP4]] : i32 -> i1
// CHECK: [[TMP6:%.+]] = moore.extract [[TMP3]] from 1 : uarray<2 x i32> -> i32
// CHECK: [[TMP7:%.+]] = moore.wildcard_eq [[TMP1]], [[TMP6]] : i32 -> i1
// CHECK: [[TMP8:%.+]] = moore.extract [[TMP2]] from 1 : uarray<2 x uarray<2 x i32>> -> uarray<2 x i32>
// CHECK: [[TMP9:%.+]] = moore.extract [[TMP8]] from 0 : uarray<2 x i32> -> i32
// CHECK: [[TMP10:%.+]] = moore.wildcard_eq [[TMP1]], [[TMP9]] : i32 -> i1
// CHECK: [[TMP11:%.+]] = moore.extract [[TMP8]] from 1 : uarray<2 x i32> -> i32
// CHECK: [[TMP12:%.+]] = moore.wildcard_eq [[TMP1]], [[TMP11]] : i32 -> i1
// CHECK: [[TMP13:%.+]] = moore.or [[TMP10]], [[TMP12]] : i1
// CHECK: [[TMP14:%.+]] = moore.or [[TMP7]], [[TMP13]] : i1
// CHECK: [[TMP15:%.+]] = moore.or [[TMP5]], [[TMP14]] : i1
c = a inside { ua2d };

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

Expand Down
4 changes: 2 additions & 2 deletions test/Conversion/ImportVerilog/errors.sv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,8 @@ endmodule

// -----
module Foo;
int a, b[3];
// expected-error @below {{unpacked arrays in 'inside' expressions not supported}}
int a, b[];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you added a lot more errors. Please add checks for them.

// expected-error @below {{unsized unpacked arrays in 'inside' expressions not supported}}
int c = a inside { b };
endmodule

Expand Down
Loading