[ExportVerilog] Handle printing of UnionExtractOp
#5022
Conversation
There was a problem hiding this comment.
LGTM. Could you add a test like this?
hw.module @Foo(%cond: i1, %a: !hw.union<foo: i2, bar: i4>, %b: !hw.union<foo: i2, bar: i4>) -> (c: i4) {
%0 = comb.mux %cond, %a, %b : !hw.union<foo: i2, bar: i4>
%1 = hw.union_extract %0["bar"] : !hw.union<foo: i2, bar: i4>
hw.output %1 : i4
}
(cond ? a : b).bar is not valid verilog so
|
I will probably close this, it doesn't handle the padding that is needed for packed unions: #5001 |
|
Yeah, the packing that SV requires is a pain in the ass. It's the reason I didn't get to verilog support two years ago! |
youngar
force-pushed
the
exportverilog-unionextract
branch
from
a6e8811
to
ab4ea78
Compare
|
Ok looks like this wasn't too bad to get working. Not super familiar with everything going on in |
Test added! |
| // CHECK-LABEL: module FooB( | ||
| // CHECK-NEXT: input union packed {logic [15:0] a; struct packed {logic [1:0] __pre_padding_b; logic [9:0] b; logic [3:0] __post_padding_b;} b;} test | ||
| // CHECK-NEXT: input union packed {logic [15:0] a; struct packed {logic [1:0] __pre_padding_b; logic [13:0] b;} b;} test, | ||
| // CHECK-NEXT: output [15:0] a, | ||
| // CHECK-NEXT: output [13:0] b | ||
| // CHECK-NEXT: ); | ||
| !unionB = !hw.union<a: i16, b: i10 offset 2> | ||
| hw.module @FooB(%test: !unionB) {} | ||
| // CHECK-EMPTY: | ||
| // CHECK-NEXT: assign a = test.a; | ||
| // CHECK-NEXT: assign b = test.b.b; | ||
| // CHECK-NEXT: endmodule | ||
| !unionB = !hw.union<a: i16, b: i14 offset 2> | ||
| hw.module @FooB(%test: !unionB) -> (a: i16, b: i14) { | ||
| %0 = hw.union_extract %test["a"] : !unionB | ||
| %1 = hw.union_extract %test["b"] : !unionB | ||
| hw.output %0, %1 : i16, i14 | ||
| } |
There was a problem hiding this comment.
I changed this test to only have pre_padding, so now we have a test with only post_padding and a test with only pre_padding.
include/circt/Support/LLVM.h
Outdated
| @@ -50,6 +50,7 @@ using mlir::dyn_cast_or_null; | |||
| using mlir::function_ref; | |||
| using mlir::isa; | |||
| using mlir::isa_and_nonnull; | |||
| using mlir::isa_and_present; | |||
There was a problem hiding this comment.
Isn't this an alias for the previous one?
There was a problem hiding this comment.
I was under the impression that the _present methods were the future and the others would be deprecated, but for this specific function I don't see a comment saying that isa_and_nonnull will be deprecated. https://discourse.llvm.org/t/psa-swapping-out-or-null-with-if-present/65018
I'll just switch it back to isa_and_nonnull, we can bulk fix these super easily in the future if we need to.
| @@ -90,7 +90,7 @@ hw.module @TESTSIMPLE(%a: i4, %b: i4, %c: i2, %cond: i1, | |||
| %46 = hw.struct_inject %45["bar"], %b : !hw.struct<foo: i2, bar: i4> | |||
| %none = hw.constant 0 : i0 | |||
| %47 = hw.array_get %array1[%none] : !hw.array<1xi1>, i0 | |||
|
|
|||
| emitError(op, "SV attributes emission is unimplemented for the op"); | ||
| emitSubExpr(op.getInput(), Selection); | ||
|
|
||
| // Check if this union type has been padded. |
There was a problem hiding this comment.
This could get expensive to compute. We might think about caching the result at some point, but that seems like pre-optimization to do now.
youngar
force-pushed
the
exportverilog-unionextract
branch
from
bae9f0d
to
3647407
Compare
No description provided.