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

[RTGTest] Add representation for immediates #8053

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

Conversation

maerhart
Copy link
Member

@maerhart maerhart commented Jan 9, 2025

No description provided.

@maerhart maerhart added the RTG Involving the `rtg` dialect label Jan 9, 2025
@maerhart maerhart requested a review from darthscsi January 9, 2025 21:03
@maerhart maerhart force-pushed the maerhart-rtgtest-improved-registers branch from 8f01c9f to 82ecbdc Compare January 9, 2025 21:26
@maerhart maerhart force-pushed the maerhart-rtgtest-immediates branch from 1809c9e to 305561c Compare January 9, 2025 21:27
@maerhart maerhart force-pushed the maerhart-rtgtest-improved-registers branch from 82ecbdc to 2f52c10 Compare January 9, 2025 21:35
@maerhart maerhart force-pushed the maerhart-rtgtest-immediates branch from 305561c to e099496 Compare January 9, 2025 21:36
@maerhart maerhart force-pushed the maerhart-rtgtest-improved-registers branch from 2f52c10 to 19ec825 Compare January 17, 2025 14:37
@maerhart maerhart force-pushed the maerhart-rtgtest-immediates branch from e099496 to 5f5636f Compare January 17, 2025 14:39
@maerhart maerhart requested a review from youngar January 17, 2025 14:39
@maerhart maerhart force-pushed the maerhart-rtgtest-improved-registers branch from 19ec825 to 610f6c0 Compare January 21, 2025 10:37
@maerhart maerhart force-pushed the maerhart-rtgtest-immediates branch from 5f5636f to 30f9e93 Compare January 21, 2025 10:38
Copy link
Contributor

@darthscsi darthscsi left a comment

Choose a reason for hiding this comment

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

Is there a major reason to use this instead of wrapping an APInt?

@maerhart
Copy link
Member Author

This is supported naturally by elaboration while just wrapping an APInt requires some more custom handling in elaboration and my goal is to avoid custom handling in elaboration when possible.

Basically, we need to know how to represent the elaborated version of the op and how to materialize it again.

  • With this representation we can just store the constant folded attribute and then call the materializer of the dialect that defines the attribute.
  • When using an APInt (i.e. IntegerAttr) we'd also need to store some additional information to know which materializer to call (e.g., the dialect of the operation that we got the folded value from and the type since the type stored in the IntegerAttr does not match the type we actually want (and was the return type of the constant op originally)). Depending on what information exactly we store additionally, we might have to internalize it in elaboration rather than storing it as a primitive value (if we just store the attribute and the type and assume that the dialect defining the type is the one with the materializer, it'd be 16 bytes and might still be fine as a primitive value).

To conclude, it's definitely possible to just wrap an APInt, but would that be more elegant than having a custom attribute? To me, this seemed more elegant, but let me know if you think otherwise.

Alternatively, it'd also be possible to just use the builtin integer type for immediates which wouldn't require any additional handling in elaboration and also avoids this entire PR, but then it's less clear when you want to define an immediate and when, e.g., an upper bound for an scf.for loop. It would mix meta and payload constructs.

Base automatically changed from maerhart-rtgtest-improved-registers to main January 24, 2025 13:47
@maerhart maerhart force-pushed the maerhart-rtgtest-immediates branch from 30f9e93 to 483e9a0 Compare January 24, 2025 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RTG Involving the `rtg` dialect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants