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

Extend Spilling #143

Merged
merged 2 commits into from
Jan 4, 2025
Merged

Extend Spilling #143

merged 2 commits into from
Jan 4, 2025

Conversation

dop-amin
Copy link
Collaborator

@dop-amin dop-amin commented Jan 3, 2025

Commits extracted from armv7m branch as the example for the 769 Dilithium NTT breaks without this.

For example, on Armv7-M microcontrollers it can be useful to spill
from the GPR file to the FPR file.

The type of this configuration option is architecture dependent.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I forgot the details here... are we always expecting this to be a dictionary of named values to be passed to the members of the Spill class?

If so, we could at least modify the getter to return {} if self._spill_type is None, removing the need for the if .. else below.

Copy link
Collaborator Author

@dop-amin dop-amin Jan 3, 2025

Choose a reason for hiding this comment

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

If I'm not mistaken, the dictionary that is passed here is supposed to be the **kwargs in the Spill class in the arch model.

In one example we set slothy.config.constraints.spill_type = { 'spill_to_vreg': 24 }, which defines the starting point of the FPRs to spill to as an integer. So yes, having a dictionary here makes sense.

Thus, I think the solution of returning {} if self._spill_type is None, which will pass no key word argument spill_to_vreg to Spill.spill(), and therefore cause a spill to the stack instead of the FPR is fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you document that we need a dict here?

Copy link
Collaborator Author

@dop-amin dop-amin Jan 3, 2025

Choose a reason for hiding this comment

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

Done 👍 It's not super precise as the option itself leaves lots of freedom in the arch model. But I think it's clearer than before.

Previously, SLOTHY would fail when a global input was spilled via
an instruction annotated with `// @slothy:is_spill`.

This commit fixes this by ad-hoc creating a VirtualInstruction node
for the input in this case -- as is done during ordinary DFG parsing
already.
@hanno-becker hanno-becker merged commit f1cad7f into slothy-optimizer:main Jan 4, 2025
13 checks passed
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.

2 participants