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

core: Add BitEnumAttribute #2617

Merged
merged 10 commits into from
Aug 15, 2024
Merged

core: Add BitEnumAttribute #2617

merged 10 commits into from
Aug 15, 2024

Conversation

zero9178
Copy link
Contributor

@zero9178 zero9178 commented May 21, 2024

The BitEnumAttribute is meant to be the equivalent of BitEnumAttr in MLIR: https://github.com/llvm/llvm-project/blob/e67f2cc3fc38cec2041cfb197ac4688ed3d16e7e/mlir/include/mlir/IR/EnumAttr.td#L266
It allows defining an attribute which in abstract is a set of flag values where each flag value is defined within an enum.

The base class is meant to be easy to use like EnumAttribute and automatically define suitable parsers and printers.
For an in-tree user that is converted by this PR see FastMathAttr in the LLVM and arith dialects.

@zero9178 zero9178 added the help wanted Extra attention is needed label May 21, 2024
Copy link

codecov bot commented May 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.84%. Comparing base (a12143e) to head (cfcbe07).
Report is 8 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2617      +/-   ##
==========================================
- Coverage   89.85%   89.84%   -0.01%     
==========================================
  Files         413      413              
  Lines       51852    51888      +36     
  Branches     8006     8011       +5     
==========================================
+ Hits        46592    46620      +28     
- Misses       3970     3975       +5     
- Partials     1290     1293       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

xdsl/ir/core.py Outdated Show resolved Hide resolved
@superlopuh
Copy link
Member

Ah, @zero9178, now it's just the formatting failing, should be easy enough to get working locally for you, then should be good to open up for review when you're ready

@AntonLydike AntonLydike changed the title [Please-take-over] core: add BitEnumAttribute [Please-take-over] core: Add BitEnumAttribute May 22, 2024
@math-fehr
Copy link
Collaborator

Currently taking over, I'm splitting this PR in multiple ones as well

@math-fehr math-fehr changed the base branch from main to fehr/parse_enum_value June 7, 2024 14:43
Base automatically changed from fehr/parse_enum_value to main June 11, 2024 16:05
@superlopuh
Copy link
Member

@math-fehr what's the latest on this?

@math-fehr
Copy link
Collaborator

Sorry about this. I merged part of it (the parsing for enums) but now I don't have time anymore to continue it in the next month.
If someone wants to take over, please do

@jorendumoulin jorendumoulin force-pushed the zero9178/bitenumattribute branch 2 times, most recently from c66c49b to 6c0a1a9 Compare August 13, 2024 14:01
@jorendumoulin jorendumoulin changed the title [Please-take-over] core: Add BitEnumAttribute core: Add BitEnumAttribute Aug 14, 2024
@jorendumoulin jorendumoulin marked this pull request as ready for review August 14, 2024 09:56
@jorendumoulin
Copy link
Collaborator

I rebased this, fixed some things and added tests and documentation. I think it's ready for review!

xdsl/ir/core.py Outdated

cls.enum_type = enum_type
super().__init__(tuple(cast(set[EnumType], flags_)))
Copy link
Member

Choose a reason for hiding this comment

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

Is the cast here necessary? it seems that all the branches above set it to the expected type

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, the branches do set it to StrEnum, set[StrEnum] does not equal set[EnumType]

Expression of type "set[StrEnum] is imcompatible with return type "set[EnumType]"

Even though EnumType = TypeVar('EnumType', bound=StrEnum)

This is because cls.enumtype is StrEnum. I cannot hovewer make this EnumType, as it is illegal to have a typevar as a classvariable

if value is None:
return None

return {cast(type[EnumType], cls.enum_type)(value)}
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Member

Choose a reason for hiding this comment

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

And below

@jorendumoulin jorendumoulin merged commit 39c774a into main Aug 15, 2024
10 checks passed
@jorendumoulin jorendumoulin deleted the zero9178/bitenumattribute branch August 15, 2024 11:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants