Skip to content
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

[MSFT] Add multicycle path op #6262

Merged
merged 2 commits into from
Oct 17, 2023
Merged

Conversation

mortbopet
Copy link
Contributor

Adds a new operation to the MSFT dialect which specifies a multicycle path constraint in between two symbols.

This implementation does not opt in to the whole dynamic instance hierarchy specification of MSFT; do we want to do that? Would have to require some thinking about what it means that a PD constraint refrences two dynamic instances, wherein msft.instance.dynamic currently expects all nested pd constraints to reference the parent instance exclusively. To this, i don't think it will be needed in our immediate usecase, anyways (assuming we're going to use hw.path.to_this which itself will generate the hw.hierpath and not use the dynamic instance hierarchy support in MSFT).

Also splits DynInstDataOpInterface into a two interfaces; one base interface (anything that pertains to dynamic instance data) and the UnaryDynInstDataOpInterface which refers to all data ops which access a single location - some additional boilerplate is introduced since the current implementation of OpInterface's doesn't seem to support inheriting interfaces implementing parent interfaces (everything is expected to be defined by the implementing op).

Exported TCL example:

proc reg_0_multicycle_config { parent } {
  set_multicycle_path -hold 1 -setup 2 -from [get_registers {$parent|reg_0}] -to [get_registers {$parent|reg2}]
}

Adds a new operation to the MSFT dialect which specifies a multicycle path constraint in between two symbols.

This implementation does not opt in to the whole dynamic instance hierarchy specification of MSFT; do we want to do that? Would have to require some thinking about what it means that a PD constraint refrences two dynamic instances, wherein `msft.instance.dynamic` currently expects all nested pd constraints to reference the parent instance exclusively.
To this, i don't think it will be needed in our immediate usecase, anyways (assuming we're going to use `hw.path.to_this` which itself will generate the `hw.hierpath` and _not_ use the dynamic instance hierarchy support in MSFT).

Also splits `DynInstDataOpInterface` into a two interfaces; one base interface (anything that pertains to dynamic instance data) and the `UnaryDynInstDataOpInterface` which refers to all data ops which access a single location - some additional boilerplate is introduced since the current implementation of OpInterface's doesn't seem to support inheriting interfaces implementing parent interfaces (everything is expected to be defined by the implementing op).

Exported TCL example:
```tcl
proc reg_0_multicycle_config { parent } {
  set_multicycle_path -hold 1 -setup 2 -from [get_registers {$parent|reg_0}] -to [get_registers {$parent|reg2}]
}
```
@@ -199,6 +205,19 @@ LogicalResult TclOutputState::emit(PDRegPhysLocationOp locs) {
return success();
}

LogicalResult TclOutputState::emit(PDMulticycleOp op) {
indent() << "set_multicycle_path ";
os << "-hold 1 ";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previous we emitted the hold and setup on different lines of SDC. Have you checked that this works with quartus?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not, but based on the documentation i see no reason how this should not work.

Copy link
Contributor

@teqdruid teqdruid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Def look harder at OpInterface inheritance.

include/circt/Dialect/MSFT/MSFTOpInterfaces.td Outdated Show resolved Hide resolved

let extraClassDeclaration = [{
// We cannot directly implement getTopModule of DynInstDataOpInterface
// since the generated .cpp will cast to the implementing class and not
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some wierdness with OpInterface inheritance. You might check out what we do with the HWModuleLike OpInterface hierarchy. Lenharth made it much deeper and fixed an upstream bug in the process.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a different situation - HWMutableModuleLike, which inherits from HWModuleLike; the former does not implement any methods of the latter, it only provides a new set of interface methods which inheriters of HWMutableModuleLike must provide in addition to HWModuleLike.

In this case, DynInstDataOpInterface defines a getTopModule interface method, which we in theory could fully implement in the inheriting interface UnaryDynInstDataOpInterface, but that is not supported.

}];
}

def PDMulticycleOp : MSFTOp<"pd.multicycle",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: dunno if I'd consider a multi-cycle path a PD optimization. If it just generates tcl (as this appears to do), it's needed for correctness (I think), in which case it's definitely not a PD optimization. Just a necessary tcl command which is being abstracted here.

I would suggest (longer term) creating a MCP op in the pipeline dialect which both results in tcl and omits the registers.

@mortbopet mortbopet merged commit c26204a into main Oct 17, 2023
5 checks passed
youngar pushed a commit to youngar/circt that referenced this pull request Oct 19, 2023
Adds a new operation to the MSFT dialect which specifies a multicycle path constraint in between two symbols.

This implementation does not opt in to the whole dynamic instance hierarchy specification of MSFT; do we want to do that? Would have to require some thinking about what it means that a PD constraint refrences two dynamic instances, wherein `msft.instance.dynamic` currently expects all nested pd constraints to reference the parent instance exclusively.
To this, i don't think it will be needed in our immediate usecase, anyways (assuming we're going to use `hw.path.to_this` which itself will generate the `hw.hierpath` and _not_ use the dynamic instance hierarchy support in MSFT).

Also splits `DynInstDataOpInterface` into a two interfaces; one base interface (anything that pertains to dynamic instance data) and the `UnaryDynInstDataOpInterface` which refers to all data ops which access a single location - some additional boilerplate is introduced since the current implementation of OpInterface's doesn't seem to support inheriting interfaces implementing parent interfaces (everything is expected to be defined by the implementing op).

Exported TCL example:
```tcl
proc reg_0_multicycle_config { parent } {
  set_multicycle_path -hold 1 -setup 2 -from [get_registers {$parent|reg_0}] -to [get_registers {$parent|reg2}]
}
```
@darthscsi darthscsi deleted the dev/mpetersen/msft_multicycle_pd branch June 4, 2024 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants