[Moore] [Canonicalizer] Lower struct-related assignOp #7341
Conversation
|
@fabianschuiki which is better? a.rewrite variableOp The problem is b. rewrite conversionOp and variableOp Then |
|
Hmmm I don't think |
In Moore, as I understand, For Ops like these, I can not read field %a if %0 is not a |
You should be able to get field "a" as follows: The |
|
@fabianschuiki |
lib/Dialect/Moore/MooreOps.cpp
Outdated
| if (auto width = memberType.getBitSize()) { | ||
| auto lowBit = rewriter.create<ConstantOp>(op.getLoc(), i32, bit); | ||
| bit += width.value(); | ||
| auto field = rewriter.create<ExtractOp>(op->getLoc(), member.type, |
There was a problem hiding this comment.
I think UnpackedStructType doesn't allow bit extraction. Is it expected way to get a field from UnpackedStructType?
There was a problem hiding this comment.
Yes it should use structExtractOp to extract a struct type.
@uenoku You are right, it was outdated. Please see these demo. |
mingzheTerapines
changed the title
mingzheTerapines
changed the title
mingzheTerapines
changed the title
Hey, @mingzheTerapines. The |
|
@hailongSun2000 I partly agree with you. |
|
For the test, it actually has two steps: Firstly Secondly |
|
I think you can just use The steps you are describing sound exactly like a mem2reg pass. Does MLIR's existing mem2reg pass potentially already do what you are describing? That would probably be easier than doing it by hand. Especially since you can't just directly replace variable ops with |
|
@Moxinilian @fabianschuiki It seems SROA Pass + Mem2Reg Pass look very similar to Canonicalize Pass. They all rewrite Ops to original value and remove unused value. What responsibility should they do? |
But we want to lower It seems not elegent |
Add func.func Dialect Disable caonicalize function for local variable
|
Mem2Reg is most useful when you have branching control flow or interaction between dialects, hence why its effect is conceptually simple here. I don't think it makes sense to use canonicalization to do what it does as it's not very local, it requires tracking many effects (notably, memory side-effects). I'm still not sure what you are trying to achieve with global variables. How would you want to eliminate a global variable? |
|
I agree with @Moxinilian: I don't think you can use a canonicalization to replicate a mem2reg pass. Variables can be read and written from many different procedures and module-level operations at the same time, and it will not be possible to find a simple value substitution for all of those situations. It's important to not treat a I think what you are trying to do is eliminate as many variables in the module and replace them with an SSA value directly. That would be a great thing to do! The Another canonicalization you could do is to try and pull assigns to // before:
%3 = moore.struct_extract_ref %0, "a"
moore.assign %3, %1
%4 = moore.struct_extract_ref %0, "b"
moore.assign %4, %2
// after:
%3 = moore.struct_create %1, %2
moore.assign %1, %3 |
|
@fabianschuiki I just tested new version of |
|
@mingzheTerapines I agree, this looks good! We could still improve the canonicalizer of |
|
@fabianschuiki We can not canonicalize to |
|
@fabianschuiki As you known, there are some problems with --moore-simplify-procedures, so it can not be enabled. But it works making canonicalizer --moore-simplify-procedures mem2reg |
| %0 = moore.constant 42 : i32 | ||
| %1 = moore.struct_extract_ref %arg0, "a" : <struct<{a: i32, b: i32}>> -> <i32> | ||
| moore.blocking_assign %1, %0 : i32 |
There was a problem hiding this comment.
It's perfect if you can add some other cases. Such as:
%0 = moore.constant 42 : i32
%1 = moore.struct_extract_ref %arg0, "a" : <struct<{a: i32, b: i32}>> -> <i32>
moore.blocking_assign %1, %0 : i32
%2 = moore.constant 43 : i32
moore.blocking_assign %1, %2 : i32
And
%0 = moore.constant 42 : i32
%1 = moore.struct_extract_ref %arg0, "a" : <struct<{a: i32, b: i32}>> -> <i32>
moore.blocking_assign %1, %0 : i32
%2 = moore.constant 43 : i32
%3 = moore.struct_extract_ref %arg0, "b" : <struct<{a: i32, b: i32}>> -> <i32>
moore.blocking_assign %2, %3 : i32
There was a problem hiding this comment.
These tests may be complicated I can print the result of your case here
%0 = moore.constant 43 : i32
%1 = moore.constant 42 : i32
%2 = moore.read %arg0 : <struct<{a: i32, b: i32}>>
%3 = moore.struct_inject %2, "a", %1 : struct<{a: i32, b: i32}>, i32
moore.blocking_assign %arg0, %3 : struct<{a: i32, b: i32}>
%4 = moore.read %arg0 : <struct<{a: i32, b: i32}>>
%5 = moore.struct_inject %4, "a", %0 : struct<{a: i32, b: i32}>, i32
moore.blocking_assign %arg0, %5 : struct<{a: i32, b: i32}>
%6 = moore.read %arg0 : <struct<{a: i32, b: i32}>>
%7 = moore.struct_inject %6, "a", %0 : struct<{a: i32, b: i32}>, i32
moore.blocking_assign %arg0, %7 : struct<{a: i32, b: i32}>
return %arg0 : !moore.ref<struct<{a: i32, b: i32}>>
and
%0 = moore.constant 43 : i32
%1 = moore.constant 42 : i32
%2 = moore.read %arg0 : <struct<{a: i32, b: i32}>>
%3 = moore.struct_inject %2, "a", %1 : struct<{a: i32, b: i32}>, i32
moore.blocking_assign %arg0, %3 : struct<{a: i32, b: i32}>
%4 = moore.read %arg0 : <struct<{a: i32, b: i32}>>
%5 = moore.struct_inject %4, "b", %0 : struct<{a: i32, b: i32}>, i32
moore.blocking_assign %arg0, %5 : struct<{a: i32, b: i32}>
return %arg0 : !moore.ref<struct<{a: i32, b: i32}>>
|
@fabianschuiki Could you please give some advice for this commit, thanks a lot. |
|
@mingzheTerapines I'm wondering if the SROA you have added to the Moore dialect doesn't more or less do a similar thing than this PR 🤔. If your goal is to get rid of subfield assignments to structs and arrays, SROA feels like a pretty good approach because it eliminates the struct altogether. Converting assignments to struct fields into assignments to the entire struct is a lot more complicated. The difficult part is that you have to make sure that assigning to the entire struct doesn't overwrite other assignments to individual fields. For example, you could have the following: struct packed {
int a;
int b;
} x;
initial x.a <= #1ns 42;
initial x.b <= #2ns 9001;This would show up like the following in the IR: %x = moore.variable
moore.procedure initial {
%x.a = moore.struct_extract_ref %x, "a"
moore.nonblocking_assign %x.a, 42 after 1ns
}
moore.procedure initial {
%x.b = moore.struct_extract_ref %x, "b"
moore.nonblocking_assign %x.b, 9001 after 2ns
}Here you'd be tempted to try and convert the assignment to the struct fields into assignments of the entire struct, like the following: moore.procedure initial {
%0 = moore.read %x
%1 = moore.struct_inject %0, "a", 42
moore.nonblocking_assign %x, %1 after 1ns
}
moore.procedure initial {
%0 = moore.read %x
%1 = moore.struct_inject %0, "b", 9001
moore.nonblocking_assign %x, %1 after 2ns
}Unfortunately this wouldn't work properly, since the two procedures would read the initial value of x, I suspect that there will be a lot of other examples where you would get a similar kind of misbehavior by simply overwriting an entire struct's value where you originally only accessed an individual field. There are a few ways to make this work though:
In any case, since this is a pretty complicated optimization that requires analysis across an entire module, this would have to be a separate transformation pass instead of a canonicalizer. |
|
@fabianschuiki Yes I found you mentioned in #7158 (comment) So I let moore.struct_inject works in moore.blocking_assign canonicalize pass, it make sense. |
|
I like the idea of using Mem2reg should offer a way to generate struct injects for assignments through subfield accesses. I'm not entirely sure how that works; you might have to declare a memory slot for every field in a struct/array? |
Using pass
canonicalizers:lower
structExtractRefOptostructInjectOpRefresh referring to struct SSA when new struct is created (structInjectOp) with pass
simplifyprocedureandmem2regupdate
union verify functionRegister
funcdialect forfunc.funcOp