[FIRRTL] Re-implement old EmitOMIR ports logic in LowerClasses. #7651
Conversation
This transformation used to exist in EmitOMIR to detect the presence of a specific field, and use it to add another specific field on the fly. While we could do this through other means downstream, for full compatibility we are re-implementing this logic. Because we are already doing many expensive passes through the object IR, this is implemented throughout the existing methods in LowerClasses, rather than adding a new pass or a new traversal to an existing pass. One consequence of this choice is we do need to perform some global mutation to, e.g., allocate new hierpath ops in a method that was previously parallelized internally to the pass. Rather than reworking things or executing that method serially, this patch adds a mutex to each instance of the pass, and uses the mutex around those global mutations. This should be a fair tradeoff, because in practice we actually only add this new field to one or two objects in the entire IR. Otherwise, this is fairly straightforward. When we are declaring classes, we check if we need to add the extra field. When we are adding class bodies, we check if we need to add the extra block argument. When we are converting object instances, we check if we need to supply the extra argument. It is in this final case that we actually make a list of objects. Now that we are off the JSON based OMIR, this is the proper way to add a new field with the same type.
| return namespaces[m]; | ||
| }); | ||
|
|
||
| portPathRef = hierPathCache.getRefFor(ArrayAttr::get(ctx, {portSym})); |
There was a problem hiding this comment.
getRefFor could create HierPathOp in a top-level module so the order could be non-deterministic due to multithreading though I agree that it would be really rare.
There was a problem hiding this comment.
Hmm yes good point. I put this section under a mutex to try to make sure it was thread safe, but I hadn't thought about non-deterministic order. I don't think it would matter in practice, but I guess in general we should strive for deterministic MLIR outputs.
I'm trying to think about how to make this deterministic... I guess when we are doing updateInstances in parallel, we can collect objects that need this extra field and process them later single-threaded. But that would still need to be smart about handling the objects that were collected from multiple threads in a stable order. Maybe I could come up with a stable sort for the collected objects and then process them serially in that order?
There was a problem hiding this comment.
I took a crack at this in 962f859. Doing the multi-threaded collection and then single-threaded post processing also helped clean up the overall diff, I think it's simpler this way.
Regarding the actual determinism, I sorted the collected objects by their class name before processing single threaded. There could still be non-determinism if we collected objects of the same class from multiple threads, but in practice this should never happen; we only add this field to a handful of objects and they should be unique within each circuit.
mikeurbach
force-pushed
the
mikeurbach/rtl-ports-lower-classes
branch
from
51e4ae4
to
962f859
Compare
|
Thanks for the reviews, I will merge this in its current form. |
This transformation used to exist in EmitOMIR to detect the presence of a specific field, and use it to add another specific field on the fly. While we could do this through other means downstream, for full compatibility we are re-implementing this logic.
Because we are already doing many expensive passes through the object IR, this is implemented throughout the existing methods in LowerClasses, rather than adding a new pass or a new traversal to an existing pass.
One consequence of this choice is we do need to perform some global mutation to, e.g., allocate new hierpath ops in a method that was previously parallelized internally to the pass. Rather than reworking things or executing that method serially, this patch adds a mutex to each instance of the pass, and uses the mutex around those global mutations. This should be a fair tradeoff, because in practice we actually only add this new field to one or two objects in the entire IR.
Otherwise, this is fairly straightforward. When we are declaring classes, we check if we need to add the extra field. When we are adding class bodies, we check if we need to add the extra block argument. When we are converting object instances, we check if we need to supply the extra argument. It is in this final case that we actually make a list of objects. Now that we are off the JSON based OMIR, this is the proper way to add a new field with the same type.