Skip to content

Conversation

@cowardsa
Copy link
Contributor

Introduce the Sklanskey Tree lowering option for comb.add and make it the default option (used in the PULP Platform library of arithmetic circuits). The Sklanskey Tree is as fast as a Kogge-Stone adder but tradesoff greater fan-out for less logic (preference will depend on target technology).

To aide testing purposes - make architecture selection optionally controlled by an attribute on the operator - in future this could be expanded to provide a way in which analysis passes could modify operator implementation selection, without interacting with the lowering pass itself. Open to suggestions if this is not a preferred method for passing "synthesis" controls?

comb.add %arg0, %arg1, %arg2 {synth.arch = "SKLANSKEY"} : i4

Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

LGTM, using attributes for testing is great idea! I want to make sure that these attributes are not part of public API so could you add explicitly include test as attribute name? like synth.test.arch maybe. I think eventually it might make sense to expose these architectures as stand-alone operations synth.add.sklanskey if an user really wants to use specific architecture in a similar way to LLVM intrinsic.

Copy link
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for making the change.

@cowardsa cowardsa merged commit 0d433e1 into llvm:main Sep 29, 2025
7 checks passed
@seldridge
Copy link
Member

This is causing nightly debug builds to timeout when running the tests added to comb-lowering-lec.mlir. See: https://github.com/llvm/circt/actions/runs/18129142989/job/51613927639 The tests seem fine, but we may need to either disable them for debug builds or modify the lit timeout.

@seldridge
Copy link
Member

☝️ I bumped the timeout to fix this in: 2bc2607

@cowardsa
Copy link
Contributor Author

Thanks for catching this @seldridge - we could also split the test across multiple files to avoid the timeout issues - then can reduce the limit back down - would that be preferable?

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.

3 participants