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

[mingzheTerapines]

For the new concept of Moore dialect, some operations will be defined as memory-related operations. Modeling memref Dialect and LLVM dialect, the operation relationship is as follows:

• ReadOp and blockingAssignOp are related to loadOp and storeOp.
• VariableOp is related to allocaOp.

However, the operations mentioned below are for basic types. This PR will support nested types in the following way:

• VariableOp with nested types is still related to allocaOp (will be replaced with structCreateOp and UnionCreateOp).
• structExtractRefOp is related to storeOp.
• structExtractOp is related to loadOp.

To implement this:

• Since these operations will be lowered to the hw dialect, the design largely refers to the hw dialect.

• Add the trait DestructurableAllocationOpInterface for VariableOp.

• Add the trait DestructurableAccessorOpInterface for structExtractOp and structExtractRefOp.

• Implement the DestructurableTypeInterface for structLikeType and the reftype of structLikeType.

For local variables:

• Use the SROA (Scalar Replacement of Aggregates) Pass to destructure all nested-type variables into basic-type variables.
• Use the Mem2Reg (Memory to Register) Pass to replace variables imported by SROA with constants.

For global/module-level variables:

• When importing Verilog, use structInjectOp rather than blockingAssignOp, because structExtractRefOp has the Destructurable trait, but global variables should not be destructured.
• structInjectOp means creating a new struct with new values and other old values.
• Use the canonicalizer Pass to fold duplicate injecting same field operations.
• Use the canonicalizer Pass to explicitly show new struct creation.
• Use the canonicalizer Pass to send source values directly and remove structExtractOp.

Also, remove some unnecessary spaces in other code.

What's more:

• Verify that the input of nested-type-related operations should match the field type defined.

To do:

• Update the use of struct SSA values referring to the latest structInjectOp SSA values.
• Design the method for union types.
• Add and support the dbg dialect to keep local variables visible after SROA & Mem2Reg.

[fabianschuiki]

I think you probably want to have the DestructurableAllocationOpInterface implemented on moore.variable for the case where the variable has a struct type. But I would keep moore.struct_create as a simple op that just returns a struct value.

It generally makes life easier in the future if ops do only one thing, and you then compose multiple ops to do more complicated things. Here for example, you'd want moore.struct_create to form a struct-typed SSA value, and moore.variable if you want to create a variable of type T and get a ref<T> back. Then, if you want a struct reference, you'd create a moore.variable of type struct and get a ref<struct> back. You can then user moore.struct_extract_ref on that ref<struct> you got back from the variable -- which means that the variable probably has to know about struct types and has to be a destructurable allocation.

[fabianschuiki]

@fabianschuiki I was wondering use structtype to limit result type of createOp. But I find hard because the result could be either of packed or unpacked.

Yeah you are right, that is indeed very annoying. I think you could create a type constraint like def AnyStructType : AnyOf<[PackedStructType, UnpackedStructType]> (or similar, I don't remember the precise syntax). But it would still be difficult to work with the struct types... I'm wondering if there might be a way to do this with LLVM's casting infrastructure: the struct types inherit from PackedType and UnpackedType, but it might be possible to make them additionally inherit from StructLikeType (which we could define separately).

Alternatively we could think about whether we actually use the PackedType and UnpackedType base classes often... If not, we might be able to change the type hierarchy such that we don't primarily distinguish between packed/unpacked.

[mingzheTerapines]

I think you probably want to have the DestructurableAllocationOpInterface implemented on moore.variable for the case where the variable has a struct type. But I would keep moore.struct_create as a simple op that just returns a struct value.

It generally makes life easier in the future if ops do only one thing, and you then compose multiple ops to do more complicated things. Here for example, you'd want moore.struct_create to form a struct-typed SSA value, and moore.variable if you want to create a variable of type T and get a ref<T> back. Then, if you want a struct reference, you'd create a moore.variable of type struct and get a ref<struct> back. You can then user moore.struct_extract_ref on that ref<struct> you got back from the variable -- which means that the variable probably has to know about struct types and has to be a destructurable allocation.

Thanks @fabianschuiki . It is very professional design. As I understanding HWAggregates Op designing, these kind of ops are all with Pure trait which means memory operations are with other Ops such as hw.wire or something.
Maybe I can also add Pure trait to moore.union/sturct Ops.
And thanks @hailongSun2000 's message, I was suggested to do memory-allocation need varify in moore.VarialbeOp, with trait DestructurableAllocationOpInterface.
I am also hoping to hear your suggestion @fabianschuiki . Thanks again!

[hailongSun2000]

I think except moore. varaible shoude display <T>, others are T.

[hailongSun2000]

After conversion pass
module {
moore.module @top() {
%x = moore.variable :
%c0_i32 = moore.constant 0 : !moore.i32
%ii = moore.struct_create(%c0_i32 , %%c0_i32) : <struct<{a: i32, b: i32}>>
moore.procedure initial {
%1 = moore.read %x :
%2 = moore.read %ii : <struct<{a: i32, b: i32}>>
%3 = moore.struct_inject %2, "a", %1, <struct<{a: i32, b: i32}>>
moore.blocking_assign %ii, %3 : <struct<{a: i32, b: i32}>> ->
%3 = moore.read %ii : <struct<{a: i32, b: i32}>>
%4 = moore.struct_extract %3, "b" : struct<{a: i32, b: i32}> -> i32
moore.blocking_assign %x, %4 : <struct<{a: i32, b: i32}>> ->
}
moore.output
}
}

If %ii doesn't have an initial value, maybe we can lower it into %ii = moore.struct_create : <struct<{a: i32, b: i32}>>
For

 %2 = moore.read %ii : <struct<{a: i32, b: i32}>>  
 %3 = moore.struct_inject %2, "a", %1, <struct<{a: i32, b: i32}>> 

Maybe we can keep the form the same as before the conversion. Like %1 = moore.struct_inject %ii, "a", %0, struct<{a: i32, b: i32}>

[hailongSun2000]

And I think we can lower moore.read into llhd.prb, meanwhile, we need to lower moore.variable into llhd.sig.
I'm not sure whether we exactly need something like moore.blocking_assign %ii, %1 : <!moore.packed<struct<{a: i32, b: i32}>>> -> <i32>. I'm also curious why hw.struct_inject has the trait of pure. I think hw.struct_inject will modify the value of its member variables. Maybe I have misunderstood something.

[fabianschuiki]

I'm also curious why hw.struct_inject has the trait of pure. I think hw.struct_inject will modify the value of its member variables. Maybe I have misunderstood something.

hw.struct_inject can be pure because it does not technically modify the fields of its input struct, but instead it copies the input struct, modifies a field, and then returns the new struct. So roughly like this:

%oldStruct = hw.struct_create("x": %a, "y": %b)
%newStruct = hw.struct_inject %oldStruct, "y", %c
// oldStruct remains unchanged, newStruct looks like this:
// %newStruct = hw.struct_create("x": %a, "y": %c)

And I agree with you: we should add the Pure trait to moore.struct_inject as well (and the union modification ops), since these never mutate the input struct (and they also can't operate on a ref<struct> type). If you wanted to "inject" a value into a struct variable, you'd use moore.struct_extract_ref to get a ref to the field, and then use one of the moore.*assign ops to assign to that field ref 😃

[mingzheTerapines]

@fabianschuiki But hw dialect does not have the op like union_inject, which is weired.

[fabianschuiki]

That is probably because unions don't really have fields that you want to change individually. The fields in a union are just different ways to look at the exact same data. Assigning a union's field is the same thing as replacing the union with a new union which has this field set. So instead of union_inject you'd just do union_create again 🙂

[hailongSun2000]

And I agree with you: we should add the Pure trait to moore.struct_inject as well (and the union modification ops), since these never mutate the input struct (and they also can't operate on a ref<struct> type). If you wanted to "inject" a value into a struct variable, you'd use moore.struct_extract_ref to get a ref to the field, and then use one of the moore.*assign ops to assign to that field ref 😃

Thanks!

[mingzheTerapines]

@fabianschuiki It seems it is not creating new struct when lowering to LLVM, refering to circt/test/Conversion/HWToLLVM/convert_aggregates.mlir

[fabianschuiki]

@mingzheTerapines It seems it is not creating new struct when lowering to LLVM

Yes, it lowers directly to the corresponding LLVM {extract,insert}value operations. What kind of LLVM did you expect to be generated?

[fabianschuiki]

Doing SROA for module-level variables sounds like a reasonable thing to do! Or maybe that's not even necessary, and Mem2Reg can create the corresponding moore.struct_create ops instead? I'm not too familiar with the detailed implementation of SROA and Mem2Reg. Pretty sure moore.struct_inject doesn't really work for module-level variables, since the assignments in a module all occur in parallel, so there is no order in the sense of old struct before the assignment and new struct after the assignment.

I wouldn't be too concerned about the naming of variables and structs: the moore.variable and moore.net ops are more about how we represent variables in the IR, and not what the variables were that the user typed. So if we have to split a single struct variable up into 100 separate variables, that is totally fine and the user will never see the names of those variables. Instead, once we care about preserving the variable names of the original Verilog source code, we'll want to start adding dbg.variable and dbg.struct and dbg.array operations in the ImportVerilog code (at least when some form of debug information is enabled). These dbg.* operations are designed to track what the original variables were in the source language, and they allow dialects like Moore and HW/Comb/Seq/Arc/FIRRTL to aggressively restructure the IR and the declarations without having to worry about maintaining any user-defined structure 😃

[mingzheTerapines]

@mingzheTerapines It seems it is not creating new struct when lowering to LLVM

Yes, it lowers directly to the corresponding LLVM {extract,insert}value operations. What kind of LLVM did you expect to be generated?

It may be lowered to llvm.mlir.undef : !llvm.struct<(i32, i2, i1)> which really create a struct
As Hailong reminded, canonicalize can convert hw.struct_inject to struct_create explictly which will be lowered to like this.

// CHECK-NEXT: [[V3:%.*]] = llvm.mlir.undef : !llvm.struct<(i32, i2, i1)>
  // CHECK-NEXT: [[V4:%.*]] = llvm.insertvalue %arg0, [[V3]][0] : !llvm.struct<(i32, i2, i1)>
  // CHECK-NEXT: [[V5:%.*]] = llvm.insertvalue %arg4, [[V4]][1] : !llvm.struct<(i32, i2, i1)>
  // CHECK-NEXT: [[V6:%.*]] = llvm.insertvalue %arg3, [[V5]][2] : !llvm.struct<(i32, i2, i1)>
  %3 = hw.struct_create (%arg3, %arg4, %arg0) : !hw.struct<foo: i1, bar: i2, baz: i32>

and this new struct will replace use with old struct
So here is my proposal:
generate moore.struct_inject for struct whose parent is SVModule Op and moore.blockingassignOp for the rest. Done
Replace struct_inject Op with struct_createOp by the Pass canonicalization like hw Dialect did. Done - reffering to canonicalize.mlir
Still need a Pass to do following job WIP
import verilog

  moore.module @Foo() {
    %x = moore.variable : <i32>
    %y = moore.variable : <i32>
    %z = moore.variable : <i32>
    %ii = moore.variable : <struct<{a: i32, b: i32}>>
    %0 = moore.constant 4 : i32
    %1 = moore.conversion %0 : !moore.i32 -> !moore.i32
    %2 = moore.struct_inject %ii, "b", %1 : !moore.ref<struct<{a: i32, b: i32}>>
    %3 = moore.read %x : i32
    %4 = moore.constant 1 : i32
    %5 = moore.add %3, %4 : i32
    %6 = moore.struct_inject %ii, "a", %5 : !moore.ref<struct<{a: i32, b: i32}>>
    %7 = moore.struct_extract %ii, "a" : <struct<{a: i32, b: i32}>> -> i32
    moore.assign %y, %7 : i32
    %8 = moore.struct_extract %ii, "a" : <struct<{a: i32, b: i32}>> -> i32
    moore.assign %z, %8 : i32
    moore.output
  }

Some pass to construct struct inject chain.

  moore.module @Foo() {
    %x = moore.variable : <i32>
    %y = moore.variable : <i32>
    %z = moore.variable : <i32>
    %ii = moore.variable : <struct<{a: i32, b: i32}>>
    %0 = moore.constant 4 : i32
    %1 = moore.conversion %0 : !moore.i32 -> !moore.i32
    %2 = moore.struct_inject %ii, "b", %1 : !moore.ref<struct<{a: i32, b: i32}>>
    %3 = moore.read %x : i32
    %4 = moore.constant 1 : i32
    %5 = moore.add %3, %4 : i32
    %6 = moore.struct_inject %2, "a", %5 : !moore.ref<struct<{a: i32, b: i32}>>
    %7 = moore.struct_extract %6, "a" : <struct<{a: i32, b: i32}>> -> i32
    moore.assign %y, %7 : i32
    %8 = moore.struct_extract %6, "a" : <struct<{a: i32, b: i32}>> -> i32
    moore.assign %z, %8 : i32
    moore.output
  }

[fabianschuiki]

Nice! Can you add a test for the folder of the StructInjectOp? I was wondering whether we have folders and constant materialization for structs already. It would be nice to have 😃

[mingzheTerapines]

Nice! Can you add a test for the folder of the StructInjectOp? I was wondering whether we have folders and constant materialization for structs already. It would be nice to have 😃

Yes, I have added relating test for struct_inject folding test. This test will fold duplicate struct injecting to the same field.
Also I supported struct_createOp struct_extractOp folding. This test will fold middle assignment for value passing.
Unused variable will be removed if unused, but assigning the whole value to a struct variable is not supported yet. So I set it as the output of module so that relating Ops will no be removed.

[fabianschuiki]

This looks nice! Thanks for doing all this hard work 🥳 Just a minor comment on a bit of possibly unneeded code.