[ImportVerilog] Support member-access expression #7039

mingzheTerapines · 2024-05-15T07:28:09Z

Support member-access expression.
Add container multiSymbolValue for multi-symbols pointing one value.

Support member-access expression. Add container multiSymbolValue for multi-symbols pointing one value. Signed-off-by: mingzheTerapines <mingzhe.zhang@terapines.com>

lib/Conversion/ImportVerilog/ImportVerilogInternals.h

hailongSun2000 · 2024-05-15T07:42:44Z

nit: Maybe you should add some error tests.

Separate two containers and their annotations.

cepheus69 · 2024-05-15T08:04:20Z

I think more tests could be added around Union / Struct（packed / unpacked）/ Enum types which would probably need member-access feature. If it is unsupported currently, adding the test into error tests might be better.

lib/Conversion/ImportVerilog/Expressions.cpp

use auto instead of const slang::ast::Expression * declare concatName with expr.member.name

Simplfy string allocation.

The signing of unpacked structures is not allowed.- IEEE Standard

mingzheTerapines · 2024-05-15T09:26:22Z

nit: Maybe you should add some error tests.

The signing of unpacked structures is not allowed. -IEEE standards
So I add two kind of unpacked structures with signed/unsigned for error examples.

Add packed unsigned struct occasion for testing.

mingzheTerapines · 2024-05-15T09:38:09Z

I think more tests could be added around Union / Struct（packed / unpacked）/ Enum types which would probably need member-access feature. If it is unsupported currently, adding the test into error tests might be better.
Seems very good advice.
struct packed/unpacked signed/unsigned tests have been added.
Can other commits may support union and enum types?

Support Union Type Modify uniont tyep to event type as error type example.

Add error example for unpacked union.

mingzheTerapines · 2024-05-16T08:41:58Z

@cepheus69 @hailongSun2000 I also supported union type, I was hoping you can review again and give some advice.

mingzheTerapines · 2024-05-16T09:26:14Z

Dear @fabianschuiki , I added

using ValueMultiSymbols =
      std::map<llvm::SmallVector<const slang::ast::ValueSymbol *>, Value>;
  ValueMultiSymbols valueMultiSymbols

Because I can not change

 using ValueSymbols =
      llvm::ScopedHashTable<const slang::ast::ValueSymbol *, Value>;
  using ValueSymbolScope = ValueSymbols::ScopeTy;
  ValueSymbols valueSymbols;

to

 using ValueSymbols =
      llvm::ScopedHashTable<[const slang::ast::ValueSymbol *](llvm::SmallVector<const slang::ast::ValueSymbol *>), Value>;
  using ValueSymbolScope = ValueSymbols::ScopeTy;
  ValueSymbols valueSymbols;

because scopedhashtable 's key can not be set as a llvm::smallvector.
Here comes another problem, this map can not be constructed or destructed with the scope of the code, which is not proper. How can I sovle this problem, plz help. =)

uenoku

I think Moore is missing struct supports (and arrays) so probably we need to add StructFieldAccess op or something.

  std::map<llvm::SmallVector<const slang::ast::ValueSymbol *>, Value>;

I think we continue using the original data structure. Member-access expression doesn't create a new value symbol so I think we should just track the root expression.

uenoku · 2024-05-16T11:19:40Z

lib/Conversion/ImportVerilog/Expressions.cpp

+    while (true) {
+      std::string structName((*exprIt).getSymbolReference()->name);
+      concatName = structName + "." + concatName;
+      symbolVec.push_back(static_cast<const slang::ast::ValueSymbol *>(


I think we could create SturctFieldAccessOp here.

Thanks for your reply, I will consider.

fabianschuiki · 2024-05-16T21:49:42Z

Thanks for working on this @mingzheTerapines! I agree with @uenoku: it would be good to define a struct field access op that can represent accesses into struct fields. Something like %field = moore.struct_field %structValue, "fieldName".

In one of your examples from the LLVM Discourse, you had the following SystemVerilog:

always_comb begin
  typedef struct packed signed {int a, b;} int64;
  int64 ii;
  int64 bb;
  ii.b = x;
  bb.b = x;
end

After ImportVerilog I'd expect there to be only 2 variables, ii and bb, and for the IR to look something like this:

moore.procedure always_comb {
  %ii = moore.variable  : !moore.packed<struct<{a: i32, b: i32}>>
  %bb = moore.variable  : !moore.packed<struct<{a: i32, b: i32}>>
  %0 = moore.struct_field %ii, "b" : !moore.packed<struct<{a: i32, b: i32}>> -> !moore.i32
  moore.blocking_assign %0, %x : !moore.i32
  %1 = moore.struct_field %bb, "b" : !moore.packed<struct<{a: i32, b: i32}>> -> !moore.i32
  moore.blocking_assign %1, %x : !moore.i32
}

The MooreToCore pass would then probably resolve these assignments to struct_field ops by using some sort of struct_inject or struct_create operation that updates an existing struct value. Something like:

moore.procedure always_comb {
  // moore.variable becomes:
  %ii0 = moore.struct_create {a: %0, b: %0} : !moore.packed<struct<{a: i32, b: i32}>>
  // moore.struct_field and blocking_assign become:
  %ii1 = moore.struct_inject %ii0, "b", %x : !moore.packed<struct<{a: i32, b: i32}>>
}

If you'd like some inspiration for struct ops, you may want to look at the HW dialect which defines a bunch of ops to create structs, access fields, etc.

mingzheTerapines · 2024-05-17T07:15:35Z

Thanks for working on this @mingzheTerapines! I agree with @uenoku: it would be good to define a struct field access op that can represent accesses into struct fields. Something like %field = moore.struct_field %structValue, "fieldName".

In one of your examples from the LLVM Discourse, you had the following SystemVerilog:
always_comb begin
  typedef struct packed signed {int a, b;} int64;
  int64 ii;
  int64 bb;
  ii.b = x;
  bb.b = x;
end
After ImportVerilog I'd expect there to be only 2 variables, ii and bb, and for the IR to look something like this:
moore.procedure always_comb {
  %ii = moore.variable  : !moore.packed<struct<{a: i32, b: i32}>>
  %bb = moore.variable  : !moore.packed<struct<{a: i32, b: i32}>>
  %0 = moore.struct_field %ii, "b" : !moore.packed<struct<{a: i32, b: i32}>> -> !moore.i32
  moore.blocking_assign %0, %x : !moore.i32
  %1 = moore.struct_field %bb, "b" : !moore.packed<struct<{a: i32, b: i32}>> -> !moore.i32
  moore.blocking_assign %1, %x : !moore.i32
}
The MooreToCore pass would then probably resolve these assignments to struct_field ops by using some sort of struct_inject or struct_create operation that updates an existing struct value. Something like:
moore.procedure always_comb {
  // moore.variable becomes:
  %ii0 = moore.struct_create {a: %0, b: %0} : !moore.packed<struct<{a: i32, b: i32}>>
  // moore.struct_field and blocking_assign become:
  %ii1 = moore.struct_inject %ii0, "b", %x : !moore.packed<struct<{a: i32, b: i32}>>
}
If you'd like some inspiration for struct ops, you may want to look at the HW dialect which defines a bunch of ops to create structs, access fields, etc.

@fabianschuiki Sorry, I don't understand. Did you mean I need to construct three ops: moore.struct_field, moore.struct_create and moore.struct_inject, and imporve MooreToCore pass to resolove moore.struct_field op to moore.struct_create and moore.struct_inject this two ops?
But moore.struct_create and moore.struct_inject this two ops look the same ops in HW dialect. Is that correct?

hailongSun2000 · 2024-05-17T09:05:49Z

@mingzheTerapines I think what @fabianschuiki means is defining struct_inject or struct_create like hw dialect. The former is to replace a field with a value in an existing one and so on. Then we can lower them into hw one-to-one.

Define a struct field access op that can represent accesses into struct fields.

mingzheTerapines · 2024-05-20T06:22:10Z

Thanks for working on this @mingzheTerapines! I agree with @uenoku: it would be good to define a struct field access op that can represent accesses into struct fields. Something like %field = moore.struct_field %structValue, "fieldName".
In one of your examples from the LLVM Discourse, you had the following SystemVerilog:
always_comb begin
  typedef struct packed signed {int a, b;} int64;
  int64 ii;
  int64 bb;
  ii.b = x;
  bb.b = x;
end
After ImportVerilog I'd expect there to be only 2 variables, ii and bb, and for the IR to look something like this:
moore.procedure always_comb {
  %ii = moore.variable  : !moore.packed<struct<{a: i32, b: i32}>>
  %bb = moore.variable  : !moore.packed<struct<{a: i32, b: i32}>>
  %0 = moore.struct_field %ii, "b" : !moore.packed<struct<{a: i32, b: i32}>> -> !moore.i32
  moore.blocking_assign %0, %x : !moore.i32
  %1 = moore.struct_field %bb, "b" : !moore.packed<struct<{a: i32, b: i32}>> -> !moore.i32
  moore.blocking_assign %1, %x : !moore.i32
}
The MooreToCore pass would then probably resolve these assignments to struct_field ops by using some sort of struct_inject or struct_create operation that updates an existing struct value. Something like:
moore.procedure always_comb {
  // moore.variable becomes:
  %ii0 = moore.struct_create {a: %0, b: %0} : !moore.packed<struct<{a: i32, b: i32}>>
  // moore.struct_field and blocking_assign become:
  %ii1 = moore.struct_inject %ii0, "b", %x : !moore.packed<struct<{a: i32, b: i32}>>
}
If you'd like some inspiration for struct ops, you may want to look at the HW dialect which defines a bunch of ops to create structs, access fields, etc.
@fabianschuiki Sorry, I don't understand. Did you mean I need to construct three ops: moore.struct_field, moore.struct_create and moore.struct_inject, and imporve MooreToCore pass to resolove moore.struct_field op to moore.struct_create and moore.struct_inject this two ops? But moore.struct_create and moore.struct_inject this two ops look the same ops in HW dialect. Is that correct?

moore.procedure always_comb {
  // moore.variable becomes:
  %ii0 = moore.struct_create {a: %0, b: %0} : !moore.packed<struct<{a: i32, b: i32}>>
  // moore.struct_field and blocking_assign become:
  %ii1 = moore.struct_inject %ii0, "b", %x : !moore.packed<struct<{a: i32, b: i32}>>
}

Seems this may not tell core(HW) this expression miss the meaning if this expression is blocking. But HW dialect do not have this kind of concern. Shall I use moore.struct_inject only one expression or I use moore.struct_blocking_inject and moore_nonblocking_inject two ops?

fabianschuiki · 2024-05-20T21:31:49Z

I would keep the blocking/non-blocking aspect out of the new op, and just have a moore.struct_inject which takes an existing SSA struct value and updates one of its fields, and returns the updated struct SSA value as a result.

Passes like Mem2Reg can then use this new op to convert assignments to individual struct fields into assignments of the entire struct. For example, the IR might initially look like this:

moore.procedure always_comb {
  // struct packed { ... } y;
  %y = moore.variable : !moore.packed<struct<{a: i32, b: i32}>>
  // y.a = x;
  %0 = moore.struct_field %y, "a" : !moore.packed<struct<{a: i32, b: i32}>> -> !moore.i32
  moore.blocking_assign %0, %x : !moore.i32
}

The Mem2Reg pass, or MooreToCore, or a dedicated assignment coarsening pass, can then convert the assignment to struct field y.a into an assignment to the entire struct y, with the new value for field a injected:

moore.procedure always_comb {
  %y = moore.variable : !moore.packed<struct<{a: i32, b: i32}>>
  %0 = moore.struct_inject %y, "a", %x, !moore.packed<struct<{a: i32, b: i32}>>
  moore.blocking_assign %y, %0 : !moore.packed<struct<{a: i32, b: i32}>>
}

This turns the subfield assignment y.a = ... into an assignment to the entire variable y = ... by dealing with the field "a" on the right-hand side instead of the left-hand side. The Mem2Reg pass can then promote the y variable to an SSA value as per usual:

moore.procedure always_comb {
  %c0_i32 = moore.constant 0 : !moore.i32
  %y0 = moore.struct_create {a: %c0_i32, b: %c0_i32} : !moore.packed<struct<{a: i32, b: i32}>>
  %y1 = moore.struct_inject %y0, "a", %x, !moore.packed<struct<{a: i32, b: i32}>>
}

I'm pretty sure that Mem2Reg can resolve these struct field assignments directly: there is a DestructurableAllocationOpInterface that allows you to declare separate memory slots for every single field in a struct or array, which the corresponding DestructurableAccessorOpInterface implemented on the struct_field op can access and resolve.

By defining a struct_field, struct_inject, and struct_create op we'll likely be able to directly use the Mem2Reg pass, or write a similar pass dedicated to the Moore dialect.

mingzheTerapines · 2024-05-21T03:22:58Z

Thanks @fabianschuiki , I already directly rewrite code to

moore.procedure always_comb {
  %y = moore.variable : !moore.packed<struct<{a: i32, b: i32}>>
  %0 = moore.struct_inject %y, "a", %x, !moore.packed<struct<{a: i32, b: i32}>>
  moore.blocking_assign %y, %0 : !moore.packed<struct<{a: i32, b: i32}>>
}

in this PR.
And I will write pass to do transform like

moore.procedure always_comb {
  %c0_i32 = moore.constant 0 : !moore.i32
  %y0 = moore.struct_create {a: %c0_i32, b: %c0_i32} : !moore.packed<struct<{a: i32, b: i32}>>
  %y1 = moore.struct_inject %y0, "a", %x, !moore.packed<struct<{a: i32, b: i32}>>
}

in another PR.

Add struct inject and extract op. Remove union support.

fabianschuiki · 2024-05-21T17:37:17Z

Hmmm, I think in ImportVerilog you want to simply create a struct_field op on the left hand side of the assignment -- basically what you had before.

ImportVerilog should be a very lightweight import pass which doesn't do a lot of conversion or lowering along the way. Converting from y.a = x to y = '{a: x, b: previous_value} is a tricky transformation that depends heavily on the kind of assignment, of where you are in the control flow, and may have very subtle interactions with when variables are read or written, and whether the struct you are assigning to is packed or unpacked. It's better if ImportVerilog captures what's in the AST here, namely that there is a struct field access on the LHS of the assignment, and leaves the lowering of these assignments to struct injects for a later pass. It's very likely that Mem2Reg will do this for you later on.

mingzheTerapines · 2024-05-22T01:20:50Z

@fabianschuiki I am concerning this kind of one-step transforming becoming two-step transforming will cost more space and timing while compiling, also may mismatch some sentences. But I am not sure. If you are sure this method is proper, I will think about it. =)

mingzheTerapines · 2024-05-22T02:42:39Z

@fabianschuiki Mem2reg is another pass, maybe I will submit the rest in another PR.

fabianschuiki

Cool this looks really nice! Thanks for working on this, and for also testing multiple nested struct accesses 🥳.

I wouldn't worry too much about performance at this stage (beyond obvious scalability issues and performance issues in the code itself). It's much more important to have a correct pipeline to go from Verilog to the core dialects, and to make the pipeline consist of orthogonal, independent passes that slowly lower the Verilog input.

Generally, you want the import conversion (ImportVerilog in this case) to mainly map from an external representation such as the Slang AST to MLIR ops, without doing any significant rewriting, lowering, or other processing. And you want the export conversion (MooreToCore in this case) to mainly translate from ops in the Moore dialect to the core dialect, without doing any significant rewriting, lowering, or other processing. All the actual lowering work you want to have in separate, isolated passes that are scheduled in between ImportVerilog and MooreToCore. This could include Mem2Reg, variable removal, pattern-matching always blocks and converting them to registers, etc. This makes it a lot easier to develop and verify the individual passes.

There was a great talk about this at EuroLLVM 2023: https://youtu.be/hIt6J1_E21c

test/Conversion/ImportVerilog/errors.sv

[ImportVerilog] Support member-access expression

5e2ba69

Support member-access expression. Add container multiSymbolValue for multi-symbols pointing one value. Signed-off-by: mingzheTerapines <mingzhe.zhang@terapines.com>

hailongSun2000 added the Moore label May 15, 2024

hailongSun2000 reviewed May 15, 2024

View reviewed changes

lib/Conversion/ImportVerilog/ImportVerilogInternals.h Outdated Show resolved Hide resolved

[ImportVerilog] Support member-access expression

ce4c917

Separate two containers and their annotations.

hailongSun2000 reviewed May 15, 2024

View reviewed changes

lib/Conversion/ImportVerilog/Expressions.cpp Outdated Show resolved Hide resolved

lib/Conversion/ImportVerilog/Expressions.cpp Outdated Show resolved Hide resolved

lib/Conversion/ImportVerilog/Expressions.cpp Outdated Show resolved Hide resolved

hailongSun2000 requested review from fabianschuiki, uenoku and maerhart May 15, 2024 08:11

mingzheTerapines added 3 commits May 15, 2024 16:24

[ImportVerilog] Support member-access expression3

1d32648

use auto instead of const slang::ast::Expression * declare concatName with expr.member.name

[ImportVerilog] Support member-access expression4

5bfc776

Simplfy string allocation.

[ImportVerilog] Support member-access expression

7cab7df

The signing of unpacked structures is not allowed.- IEEE Standard

[ImportVerilog] Support member-access expression6

8d69676

Add packed unsigned struct occasion for testing.

mingzheTerapines added 2 commits May 16, 2024 16:32

[ImportVerilog] Support Union Type

bb02980

Support Union Type Modify uniont tyep to event type as error type example.

[ImportVerilog]Add errors example

cb7f8ca

Add error example for unpacked union.

uenoku reviewed May 16, 2024

View reviewed changes

[ImportVerilog] Add strucfield op

1017e5d

Define a struct field access op that can represent accesses into struct fields.

[ImportVerilog] Add struct inject and extract op

f6529e5

Add struct inject and extract op. Remove union support.

[ImportVerilog] Support memberaccess

6bce65f

Removed some useless include.

d26dc7f

fabianschuiki approved these changes May 22, 2024

View reviewed changes

test/Conversion/ImportVerilog/errors.sv Outdated Show resolved Hide resolved

mingzheTerapines and others added 4 commits May 23, 2024 09:25

fix test error

849c898

Fix errors.sv

875e916

Merge branch 'llvm:main' into mingzhe-dev

120491a

remove space.

538aea3

hailongSun2000 merged commit 9006a44 into llvm:main May 23, 2024
4 checks passed

mingzheTerapines deleted the mingzhe-dev branch May 23, 2024 02:52

hailongSun2000 mentioned this pull request Jun 13, 2024

[ImportVerilog] [SROA] [Mem2Reg][Canonicalizers]Support Passes for Nested Type #7158

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ImportVerilog] Support member-access expression #7039

[ImportVerilog] Support member-access expression #7039

mingzheTerapines commented May 15, 2024

hailongSun2000 commented May 15, 2024 •

edited

Loading

cepheus69 commented May 15, 2024 •

edited

Loading

mingzheTerapines commented May 15, 2024

mingzheTerapines commented May 15, 2024

mingzheTerapines commented May 16, 2024

mingzheTerapines commented May 16, 2024

uenoku left a comment

uenoku May 16, 2024

mingzheTerapines May 16, 2024

fabianschuiki commented May 16, 2024 •

edited

Loading

mingzheTerapines commented May 17, 2024

hailongSun2000 commented May 17, 2024

mingzheTerapines commented May 20, 2024

fabianschuiki commented May 20, 2024 •

edited

Loading

mingzheTerapines commented May 21, 2024 •

edited

Loading

fabianschuiki commented May 21, 2024

mingzheTerapines commented May 22, 2024

mingzheTerapines commented May 22, 2024

fabianschuiki left a comment

[ImportVerilog] Support member-access expression #7039

[ImportVerilog] Support member-access expression #7039

Conversation

mingzheTerapines commented May 15, 2024

hailongSun2000 commented May 15, 2024 • edited Loading

cepheus69 commented May 15, 2024 • edited Loading

mingzheTerapines commented May 15, 2024

mingzheTerapines commented May 15, 2024

mingzheTerapines commented May 16, 2024

mingzheTerapines commented May 16, 2024

uenoku left a comment

Choose a reason for hiding this comment

uenoku May 16, 2024

Choose a reason for hiding this comment

mingzheTerapines May 16, 2024

Choose a reason for hiding this comment

fabianschuiki commented May 16, 2024 • edited Loading

mingzheTerapines commented May 17, 2024

hailongSun2000 commented May 17, 2024

mingzheTerapines commented May 20, 2024

fabianschuiki commented May 20, 2024 • edited Loading

mingzheTerapines commented May 21, 2024 • edited Loading

fabianschuiki commented May 21, 2024

mingzheTerapines commented May 22, 2024

mingzheTerapines commented May 22, 2024

fabianschuiki left a comment

Choose a reason for hiding this comment

hailongSun2000 commented May 15, 2024 •

edited

Loading

cepheus69 commented May 15, 2024 •

edited

Loading

fabianschuiki commented May 16, 2024 •

edited

Loading

fabianschuiki commented May 20, 2024 •

edited

Loading

mingzheTerapines commented May 21, 2024 •

edited

Loading