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

[firtool] Run canonicalization before LowerLayers #7534

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

seldridge
Copy link
Member

Run canonicalization before LowerLayers in order to optimize things which are difficult to do after layerblocks have been converted to modules. The conversion to modules means that many trivial canonicalizers are now very difficult to do as they require inter-module analysis.

Ideally, it would be best if LowerLayers ran as late as possible. (Generally any outlining should run as late as possible.) However, we aren't quite ready to make this harder move of pushing LowerLayers to the end of the FIRRTL pipeline.

Fixes #7533.

Run canonicalization before LowerLayers in order to optimize things which
are difficult to do after layerblocks have been converted to modules.  The
conversion to modules means that many trivial canonicalizers are now very
difficult to do as they require inter-module analysis.

Ideally, it would be best if LowerLayers ran as late as
possible.  (Generally any outlining should run as late as possible.)
However, we aren't quite ready to make this harder move of pushing
LowerLayers to the end of the FIRRTL pipeline.

Fixes #7533.

Signed-off-by: Schuyler Eldridge <[email protected]>
@seldridge seldridge requested review from dtzSiFive and youngar August 19, 2024 23:50
@seldridge
Copy link
Member Author

Discussed offline with @dtzSiFive. There appears to be at least one register canonicalizer (see foldHiddenReset in FIRRTLFolds.cpp) and this shouldn't be run before RandomizeRegisterInit because that can cause changes in the randomization behavior due to optimizations.

It would be better to move LowerLayers later in the pipeline so that it runs after canonicalization. This would require landing #7398 to move it after ModuleInliner and see about moving it after RandomizeRegisterInit. The latter change would be beneficial as this would ensure randomization stability involving specialization that enables layers (though not guarantee it for disabling layers).

@uenoku
Copy link
Member

uenoku commented Aug 21, 2024

IIRC we have wanted to perform register optimization only in `RegisterOptimization pass so removing foldHiddenReset from canonicalizer looks fine solution to me.

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.

[FIRRTL] design with layers not optimized as well as without
2 participants