[ImportVerilog] Support member-access expression

[mingzheTerapines]

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

[hailongSun2000]

nit: Maybe you should add some error tests.

[cepheus69]

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.

[mingzheTerapines]

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.

[mingzheTerapines]

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?

[mingzheTerapines]

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

[mingzheTerapines]

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]

I think we could create SturctFieldAccessOp here.

[mingzheTerapines]

Thanks for your reply, I will consider.

[fabianschuiki]

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]

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]

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

[mingzheTerapines]

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]

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]

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.

[fabianschuiki]

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]

@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]

@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