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

Replaced 'replicate' to correctly named 'replace' flags #7442

Merged
merged 2 commits into from
Aug 6, 2024

Conversation

jpien13
Copy link
Contributor

@jpien13 jpien13 commented Aug 5, 2024

Renamed and replaced shouldReplicateSequentialMemories to shouldReplaceSequentialMemories per issue #7384

MLIR Tests:
[0/1] Running the MLIR regression tests

Testing Time: 53.62s

Total Discovered Tests: 2877
Unsupported : 506 (17.59%)
Passed : 2370 (82.38%)
Expectedly Failed: 1 (0.03%)

CIRCT Tests:
[0/1] Running the CIRCT regression tests

Testing Time: 12.50s

Total Discovered Tests: 824
Unsupported : 21 (2.55%)
Passed : 797 (96.72%)
Expectedly Failed: 6 (0.73%)

CIRCT integration tests:
[1/2] Running the CIRCT integration tests

Testing Time: 15.29s

Total Discovered Tests: 98
Unsupported : 63 (64.29%)
Passed : 34 (34.69%)
Expectedly Failed: 1 (1.02%)

@mwachs5
Copy link
Contributor

mwachs5 commented Aug 5, 2024

@jpien13 you are failing the code formatting check. See

* Please use clang-format in the LLVM style. There are good plugins
. You can just do what the C/I check says (as I just learned myself as my PR is also currently failing the check, you can click the red X for your failing test, then scroll a few lines down below the failing job and expand to see the actual formatting issues --
Screenshot 2024-08-05 at 2 16 06 PM and try to fix them manually.

Or even better you can get clang-format working in your environment, the formatting configuration file is included in the repo.

@@ -78,7 +78,7 @@ LogicalResult firtool::populateCHIRRTLToLowFIRRTL(mlir::PassManager &pm,
pm.nest<firrtl::CircuitOp>().addPass(firrtl::createInferWidthsPass());

pm.nest<firrtl::CircuitOp>().addPass(
firrtl::createMemToRegOfVecPass(opt.shouldReplicateSequentialMemories(),
firrtl::createMemToRegOfVecPass(opt.shouldReplaceSequentialMemories(),
Copy link
Contributor

Choose a reason for hiding this comment

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

the formatter doesn't like the trailing whitespace at the end of this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Megan! Yea i just saw this, i made the change just now. Had to install the clang formatter. Should be good now? We'll see what CI job says

Copy link
Contributor Author

@jpien13 jpien13 Aug 5, 2024

Choose a reason for hiding this comment

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

I'll have to doublecheck the config file, I didn't see your comments till now (before I pushed the newest change to get rid of the whitespace). I'll check my settings and formatting now. Thanks :)

**EDIT: found the .clang-format file, all set.

@mwachs5
Copy link
Contributor

mwachs5 commented Aug 6, 2024

LGTM! @youngar can this be merged? I don't have permissions to do so.

@youngar youngar added the FIRRTL Involving the `firrtl` dialect label Aug 6, 2024
@youngar youngar merged commit 1a8f82e into llvm:main Aug 6, 2024
4 checks passed
@youngar
Copy link
Member

youngar commented Aug 6, 2024

Thanks @jpien13!

@jpien13 jpien13 deleted the issue-7384 branch August 7, 2024 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FIRRTL Involving the `firrtl` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants