[Arc] Fully support initialization through seq.initial #7605
Conversation
| OpBuilder builder(reg); | ||
| auto init = builder | ||
| .create<mlir::UnrealizedConversionCastOp>( | ||
| reg.getLoc(), reg.getType(), reg.getInitialValue()) | ||
| .getResult(0); |
There was a problem hiding this comment.
I'm wondering if it might be worthwhile having an immutable<T> -> T cast operation, something like seq.unwrap_immutable. That could also enable some canonicalizations where a seq.compreg could lower to its initial value if it provably can never change.
Unrealized conversion cast also works though 😃
There was a problem hiding this comment.
Or we might reconsider supporting arc.initial before lower state and do the conversion to it in ConvertToArcs?
There was a problem hiding this comment.
seq.unwrap_immutable sounds reasonable and will follow-up. arc.initial might work but currently arc.initial doesn't produce any value and we need to allocate states for them. So I guess there would be a big overlap with LowerState. Another approach might be to change arc.state's initial value operand to take seq.immutable type instead of a normal integer.
| for (Operation &op : *module.getBodyBlock()) { | ||
| if (isa<seq::InitialOp>(&op)) { | ||
| initialOps.push_back(cast<seq::InitialOp>(&op)); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Nit: can remove braces for consistency
| @@ -236,8 +242,9 @@ hw.module @Trivial(in %clock: !seq.clock, in %i0: i4, in %reset: i1, out out: i4 | |||
|
|
|||
| // CHECK-LABEL: hw.module @TrivialWithInit( | |||
| hw.module @TrivialWithInit(in %clock: !seq.clock, in %i0: i4, in %reset: i1, out out: i4) { | |||
| // CHECK: [[CST2:%.+]] = hw.constant 2 : i4 | |||
| // CHECK: [[RES0:%.+]] = arc.state @[[TRIVIALINIT_ARC]](%i0) clock %clock reset %reset initial ([[CST2]] : i4) latency 1 {names = ["foo"] | |||
| // CHECK: %[[INIT:.+]] = seq.initial { | |||
There was a problem hiding this comment.
Nit:
| // CHECK: %[[INIT:.+]] = seq.initial { | |
| // CHECK: %[[INIT:.+]] = seq.initial { |
| // RUN: arcilator %s --run --jit-entry=main | FileCheck %s | ||
| // REQUIRES: arcilator-jit | ||
|
|
||
| // CHECK: o1 = 10 |
There was a problem hiding this comment.
Shouldn't this be 16, or am I missing something?
There was a problem hiding this comment.
circt/lib/Conversion/ArcToLLVM/LowerArcToLLVM.cpp
Lines 539 to 540 in bba8cfb
It is printed in hex but yeah, it's a bit confusing so I'll change to use a smaller number.
There was a problem hiding this comment.
Oh, totally forgot about that. Maybe we should prepend a 0x to this format string. Or a smaller number, a comment, or using 0x10 at the constant op would be nice 🙂
| OpBuilder builder(reg); | ||
| auto init = builder | ||
| .create<mlir::UnrealizedConversionCastOp>( | ||
| reg.getLoc(), reg.getType(), reg.getInitialValue()) | ||
| .getResult(0); |
There was a problem hiding this comment.
Or we might reconsider supporting arc.initial before lower state and do the conversion to it in ConvertToArcs?
uenoku
force-pushed
the
dev/hidetou/arc-init
branch
from
4d1e7ea
to
61b2ba2
Compare
|
Thank you for the review! I'm going to merge this and will follow up how to deal with unrealized conversion cast. |
This PR implements state initialization using
seq.initialnot limited to constants. Combined with #7606 we can perform random initialization in arcilator