-
Notifications
You must be signed in to change notification settings - Fork 299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Arc] Add InitialOp and lowering support for FirReg preset values. #7480
Conversation
Really cool, thanks for working on this 🥳! I like the idea of putting the initial (and maybe also final) stuff into a separate region and block. I'm wondering though if it's worth the hassle of adding a separate region to A benefit could be that the You're right though about |
That sounds a lot like what I did yesterday and how I've managed the lowering of I kind of like the controlflow-ish expressiveness of the
For raw binary images where we would just copy them into the storage I'd agree. But the common Thanks a lot for your feedback. I'll see what I can come up with over the next couple of days. |
Really cool! Thank you for working on this! Using attributes as initial values would be too restrictive for most of practical use cases such as memory initialization with I'm currently working on similar change for seq dialect side in order to explicitly initialize registers with potentially side-effecting ops (such as %dpi, %random = seq.initial {
%0 = sim.call @foo : i32
%1 = = sv.call @rand: i32
seq.yield %0, %1: i32, i32
} : !seq.immutable<i32>, !seq.immutable<i32> // types to indicate values are timing-invariant.
%a = seq.compreg %clock, reset %reset, initial %dpi_call : (seq.clock, i1, !seq.immutable<i32>) -> i32
%b = seq.compreg %clock, reset %reset, initial %random : (seq.clock, i1, !seq.immutable<i32>) -> i32
that will be lowered into following SV by normal firtool flow: reg [31:0] a, b;
initial begin
a = foo();
b = rand();
end I think probably we can do similar thing for Arc as well. |
Cool, thanks for sharing @uenoku! After our discussions regarding the For procedural regions in Sim I've drafted an operation which (unsurprisingly) works a lot like an Arc with latency 1:
For non-simulation flows It would be really nice to string all of this together. In the end I just want support for FIRRTL |
3815ec1
to
ee28446
Compare
The ModelOp's region is out, the InitialOp is in. Basic smoke tests for lowering the After much agonizing, I've added the initial values as SSA operands to the StateOp. To some extent I'd still prefer using a initializer region (for the StateOp, not the ModelOp) like it is done for LLVM GlobalOps. But this would make correlating initializers of different registers like in @uenoku's example difficult. I'm currently assuming that states without initializers are implicitly zero initialized. As far as I can see we are always providing a zeroed storage to the model. An |
@@ -659,11 +680,12 @@ def ModelOp : ArcOp<"model", [RegionKindInterface, IsolatedFromAbove, | |||
specifies the I/O of the module associated to this model. | |||
}]; | |||
let arguments = (ins SymbolNameAttr:$sym_name, | |||
TypeAttrOf<ModuleType>:$io); | |||
TypeAttrOf<ModuleType>:$io, | |||
OptionalAttr<FlatSymbolRefAttr>:$initialFn); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SymbolUserOpInterface is necessary if we want to add initializer as a function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Missed that one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not competent reviewer for Arc but the design of initial op looks reasonable to me. Thank you for working on this. I'm a bit worried about adding "initialize 0 by default" logic to several places so in the future it would be nice to isolate the default initialization to a single pass.
Yeah, we should probably add |
Right - I think this is a good point to draw a line and wait for @fabianschuiki and/or @maerhart to point out my blind spots. 😬 The basic lowering for a
After StripSV:
After ConvertToArcs:
After LowerState:
After LowerClocksToFuncs:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on that! Such a cool new arcilator feature 🎉
include/circt/Dialect/Arc/ArcOps.td
Outdated
let extraClassDeclaration = [{ | ||
mlir::Block &getBodyBlock() { return getBody().front(); } | ||
}]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SingleBlock
trait auto-generates this if you also rename the body
region to bodyRegion
(or similar).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks. This was just copy pasted from the PassThroughOp, so I guess we could add SingleBlock
there, too.
include/circt/Dialect/Arc/ArcOps.td
Outdated
mlir::Block &getBodyBlock() { return getBody().front(); } | ||
}]; | ||
let builders = [ | ||
OpBuilder<(ins), "build($_builder, $_state, ValueRange{});"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this builder is not generated by default? What would the ValueRange do if we'd pass one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right That was a left-over from a point where I had InitialOp IsolatedFromAbove
and gave it an argument list.
include/circt/Dialect/Arc/ArcOps.td
Outdated
@@ -452,6 +460,20 @@ def PassThroughOp : ArcOp<"passthrough", [NoTerminator, NoRegionArguments]> { | |||
}]; | |||
} | |||
|
|||
def InitialOp : ArcOp<"initial", [RecursiveMemoryEffects, NoTerminator, NoRegionArguments]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add HasParent<"ModelOp">
or are there reasons why we want the additional flexibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to restrict it. Again, probably same for PassThroughOp?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! I think it also makes sense to add it for PassThroughOp.
include/circt/Dialect/Arc/ArcOps.td
Outdated
@@ -452,6 +460,20 @@ def PassThroughOp : ArcOp<"passthrough", [NoTerminator, NoRegionArguments]> { | |||
}]; | |||
} | |||
|
|||
def InitialOp : ArcOp<"initial", [RecursiveMemoryEffects, NoTerminator, NoRegionArguments]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks looks like more than 80 cols to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can proudly announce that I have now enabled the vertical ruler for TableGen files in my editor.
include/circt/Dialect/Arc/ArcOps.td
Outdated
@@ -652,18 +674,20 @@ def TapOp : ArcOp<"tap"> { | |||
} | |||
|
|||
def ModelOp : ArcOp<"model", [RegionKindInterface, IsolatedFromAbove, | |||
NoTerminator, Symbol]> { | |||
NoTerminator, Symbol, | |||
DeclareOpInterfaceMethods<SymbolUserOpInterface>]> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More than 80 cols?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. 😞
assert(!modelOp.getInitialFn() && | ||
"Model should not have an initializer at this point."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this emit a proper error message because I think the user could write up an input file that triggers this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've turned it into a warning. We could be very thorough and add a call from the generated initializer to the preexisting one. But I don't really see a use case for that.
} else if (isa<InitialOp>(clockOp)) { | ||
assert(!modelOp.getInitialFn() && | ||
"Model should not have an initializer at this point."); | ||
modelOp.setInitialFnAttr(FlatSymbolRefAttr::get(funcOp.getSymNameAttr())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if there are multiple initial
blocks? Do we merge them before this pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now I assume only one exists. But I don't have a strong opinion on this and merging them should be trivial. Maybe we should add a region verifier to the ModelOp, that checks the number of InitialOps (and PassThroughOps)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only allowing one sounds like a good idea to me. I think in general there is nothing wrong with having multiple ones, so maybe just check it in this pass and emit an error that it is not supported (yet) by the pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is a nice starting point! We can always relax this and collect multiple arc.initial
ops into the init function later on.
funcName.clear(); | ||
funcName.append(modelOp.getName()); | ||
funcName.append("_passthrough"); | ||
builder.create<func::CallOp>(clockOp->getLoc(), funcName, TypeRange{}, | ||
ValueRange{funcOp.getBody().getArgument(0)}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A passthrough function is only generated if a arc.passthrough
op was present. Should we keep track of whether that was the case and only insert this call in that case? Or should we generate a dummy passthrough function in this pass even if no passthrough op was present? Personally, I'd opt for the earlier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. It should now work without a PassThroughOp.
builder.create<func::CallOp>(clockOp->getLoc(), funcOp, | ||
ValueRange{modelStorageArg}); | ||
} else if (isa<InitialOp>(clockOp)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: maybe use a TypeSwitch?
) | ||
if model.hasInitialFn: | ||
print(f" {model.name}_initial(&storage[0]);") | ||
print(" }") | ||
print(f" void eval() {{ {model.name}_eval(&storage[0]); }}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for also updating this python script!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool!
Many thanks for the detailed review, @maerhart . |
LowerClocksToFuncs now checks that there is no more than one InitialOp and PassThroughOp in the model. I've also taken the liberty to move the ODS traits for ClockTreeOp, InitialOp, and PassThroughOp to a common ClockTreeLikeOp class. Compared to before this adds |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fantastic! Thanks for all the diligent checks, tests, and error messages that this adds. I also like your choice of using an SSA value as the initial value, since it isn't entirely clear yet how isolated the initializers are going to be in practice.
Can you check if the Rocket core in arc-tests
still runs with your changes? I don't see why not, but just as a sanity check. Rocket should be updated to work with newer firtool versions. Boom probably doesn't work with current firtool anymore.
(`reset` $reset^)? `latency` $latency attr-dict | ||
`:` functional-type($inputs, results) | ||
(`reset` $reset^)? | ||
( `initial` ` ` `(` $initials^ `:` type($initials) `)`)? | ||
`latency` $latency attr-dict `:` functional-type($inputs, results) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could a OptionalTypesMatchWith
constraint between the $initials
types and $results
types make the : type($initials)
part redundant? Would be cool if we could get the generated parser to infer the types based on the results 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OptionalTypesMatchWith
sadly doesn't appear to work with type ranges. RangedTypesMatchWith
on the other hand refuses to infer type($initials)
even if I make it non-optional. Maybe it cannot infer an operand range from a result range?!? ODS continues to surprise me for better or worse. I can still imagine that it could be done somehow, but at that point I had thrown in the towel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah there might be something about results having to be derived from operands to make ODS do the magic automatically. Thanks for trying! 🥳
} | ||
return | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is really fantastic!
arc.initial { | ||
%cst0 = llvm.mlir.constant(0 : i32) : i32 | ||
%stderr = llvm.call @_arc_env_get_print_stream(%cst0) : (i32) -> !llvm.ptr | ||
%str = llvm.mlir.addressof @global_init_str : !llvm.ptr | ||
%0 = llvm.call @_arc_libc_fputs(%str, %stderr) : (!llvm.ptr, !llvm.ptr) -> i32 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! I guess a future arc.final
op could work pretty much in the exact same way, with just a different point in time when it's called. Really neat!
auto referencedOp = | ||
symbolTable.lookupNearestSymbolFrom(*this, getInitialFnAttr()); | ||
if (!referencedOp) | ||
return emitError("Cannot find declaration of initializer function '") | ||
<< *getInitialFn() << "'."; | ||
auto funcOp = dyn_cast<func::FuncOp>(referencedOp); | ||
if (!funcOp) { | ||
auto diag = emitError("Referenced initializer must be a 'func.func' op."); | ||
diag.attachNote(referencedOp->getLoc()) << "Initializer declared here:"; | ||
return diag; | ||
} | ||
if (!llvm::equal(funcOp.getArgumentTypes(), getBody().getArgumentTypes())) { | ||
auto diag = emitError("Arguments of initializer function must match " | ||
"arguments of model body."); | ||
diag.attachNote(referencedOp->getLoc()) << "Initializer declared here:"; | ||
return diag; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea checking that the function signature is reasonable.
This makes me think that we might want to create something like a arc.lowered_model
op in the future which has no body region, but has symbol refs to the init, eval, and final functions, and a symbol ref to the data layout (@maerhart has been working on a prototype to move the sim memory layout into a struct-like op). That would allow us to keep all information about the generated models around until the very end, without restricting the conversion to functions and further optimizations. Anyway, very much future stuff 😉
// TODO: There are some other ops we probably want to allow | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice defensive start 🥳
// Materialize initial value, assume zero initialization as default. | ||
if (reg.getPreset() && !reg.getPreset()->isZero()) { | ||
assert(hw::type_isa<IntegerType>(reg.getType()) && | ||
"cannot lower non integer preset"); | ||
presetValue = builder.createOrFold<hw::ConstantOp>( | ||
reg.getLoc(), IntegerAttr::get(reg.getType(), *reg.getPreset())); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of having an implicit initialization value as a default, e.g. 0
for two-valued, X
for four-valued, and U
for nine-valued integers, and then only add explicit initializers if they differ. Most SV designs will not have initializers on their registers, so that will make the common case less verbose 👍
It's working, and I much appreciated you updating the FIRRTL models. Now, if we could find a way to allow the As much as I hate saying it, but we probably need some notion of the "pre-initial" state. This would at the bare minimum encompass the initially observable state of the input ports. And on top of that, there are of course the verilog peculiarities, like
*sigh* I keep meaning to do a write-up of the (non-authoritative) conclusions I've drawn when messing up my mind with the entire four/nine valued logic stuff. I just cannot find the right place or time, and this PR certainly isn't it. Again, thanks a lot for your feedback @fabianschuiki, @maerhart, @uenoku, and see you at the next PR. 😉 |
Do we have a plan to provide chisel api to the preset? |
@fzi-hielscher Very cool stuff! I definitely expect us to have to iterate on initialization order and all that quite a bit. The SV standard is very peculiar and nebulous about when some of the initialization happens. And we may want to make our own pick regarding what the exact initialization is. For example, I think it would make sense if initialization would also include initial runs of dependent always blocks and the like, but that might be totally different from other simulators. |
@uenoku has been poking at that. |
Yes, we haven't sorted out IR design for FIRRTL but this is definitely on the plate. Constant preset value could be easily added but initialization with side-effecting op like DPI requires more design work. |
An ongoing effort to carve out a path through the Arc dialect from the LLVM end towards memory and register initializers and miscellaneous stuff that may happen once at the start of simulation. Please don't look too closely at the code, yet. But I'd be happy to know if I'm on the right path or have taken a wrong turn already.
Outdated. See comments below.
The central addition so far is an
initial
region to the ArcModelOp
. which is able to modifiy thestatestorage that gets passed to the body region e.g.,:The initial values are currently derived from an attribute attached to the
AllocStateOp
. Similarly, I intend to add an attribute for file based initialization to theAllocMemoryOp
. What I have to figure out yet is how to best get these Attributes to those Ops during lowering. I'm reluctant to add even more stuff to theStateOp
, although that seems like the obvious path.