Skip to content

[CIR] Refactor IntType constraints #1593

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.

Sign up for GitHub

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

Jump to bottom
Merged
xlauko merged 1 commit into from
May 1, 2025
Merged

[CIR] Refactor IntType constraints #1593

xlauko merged 1 commit into from
May 1, 2025

Conversation

@xlauko
Copy link
Collaborator

@xlauko xlauko commented Apr 28, 2025 •
edited
Loading

  • Adds CIR_ prefixes to integer type constraints types to disambiguate their names from other dialects.
  • Renames PrimitiveInt to CIR_AnyFundamentalIntType to align more with constrian conventions.
  • Adds bunch of helper constraint classes to be able to define base types to reduce clutter of necessary type casts.
  • Reworks constraints to use CIR_ConfinedType to avoid repeating validation checks.
  • Adds IntOfWidths variadic bitwidth constraint to reduce boilerplate code needed to handle multi-bitwidth parameters.
  • Constraints are moved into a separate file, which starts decoupling of constraints and types to remove the cyclic dependency between types and attributes and will eventually help fix several outstanding TODOs.

@xlauko
Copy link
Collaborator Author

xlauko commented Apr 28, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@xlauko xlauko marked this pull request as ready for review April 28, 2025 12:22
@xlauko
Copy link
Collaborator Author

xlauko commented Apr 28, 2025

Few questions remains:

  • IntType defines maximal bitwidth 128, though all checks except verifier allows only 64. Should we bump this to 128?
  • PrimitiveInt before checked whether bitwitdh is from [8, 16, 32, 64], CIR_IntType verifier has only check that bitwidth is from range min to max bitwidth. Do we want to disallow other bitwidths in the verifier?

@xlauko xlauko force-pushed the users/xlauko/cir-type-constraints branch from 5b17612 to a8a9696 Compare April 28, 2025 13:04
@xlauko xlauko mentioned this pull request Apr 28, 2025
Merged
@xlauko xlauko force-pushed the users/xlauko/cir-type-constraints branch from a8a9696 to 04010e1 Compare April 28, 2025 13:43
@bcardosolopes
Copy link
Member

bcardosolopes commented Apr 28, 2025

  • IntType defines maximal bitwidth 128, though all checks except verifier allows only 64. Should we bump this to 128?

Yes - looks like we forgot to update. If it's possible to add one test for this that'd be great too.

  • PrimitiveInt before checked whether bitwitdh is from [8, 16, 32, 64], CIR_IntType verifier has only check that bitwidth is from range min to max bitwidth. Do we want to disallow other bitwidths in the verifier?

Yes, those instructions are known to work with the "primitive" sizes, but it's not clear whether they work for arbitrary int sizes (or that they can be lowered or have folders working, etc), so I'd rather be on the safe side - have you seen anything different while investigating? I suggest we do not change the semantic in this PR, but do that (with tests) in follow up work.

bcardosolopes
bcardosolopes requested changes Apr 28, 2025
View reviewed changes
Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

Marking as needed changes on top of discussion.

@xlauko
Copy link
Collaborator Author

xlauko commented Apr 29, 2025

Yes, those instructions are known to work with the "primitive" sizes, but it's not clear whether they work for arbitrary int sizes (or that they can be lowered or have folders working, etc), so I'd rather be on the safe side - have you seen anything different while investigating? I suggest we do not change the semantic in this PR, but do that (with tests) in follow up work.

I agree, I will fix this to mirror previous semantics, and polish operations in later run through with more testing. Though I suggest to rename Primitive to Fundamental to align with how they are called in standard? I did not like before that Primitive before suggested that non-primitive ints are somehow more complex.

@xlauko xlauko force-pushed the users/xlauko/cir-type-constraints branch 4 times, most recently from 9f0908b to fcc777a Compare April 29, 2025 17:16
@xlauko xlauko mentioned this pull request Apr 29, 2025
Merged
@xlauko xlauko force-pushed the users/xlauko/cir-type-constraints branch from fcc777a to ccc0b65 Compare April 30, 2025 11:36
@bcardosolopes
Copy link
Member

bcardosolopes commented Apr 30, 2025

Though I suggest to rename Primitive to Fundamental to align with how they are called in standard?

Sounds good, good observation on "complexity"!!

@xlauko xlauko merged commit e552f87 into main May 1, 2025
9 checks passed
xlauko added a commit to llvm/llvm-project that referenced this pull request May 1, 2025
@xlauko
[CIR] Refactor IntType constraints
04686fc 
- Adds `CIR_` prefixes to integer type constraints types to disambiguate their names from other dialects.
- Renames `PrimitiveInt` to `CIR_AnyFundamentalIntType` to align more with constrian conventions.
- Adds bunch of helper constraint classes to be able to define base types to reduce clutter of necessary type casts.
- Reworks constraints to use `CIR_ConfinedType` to avoid repeating validation checks.
- Adds `IntOfWidths` variadic bitwidth constraint to reduce boilerplate code needed to handle multi-bitwidth parameters.
- Constraints are moved into a separate file, which starts decoupling of constraints and types to remove the cyclic dependency between types and attributes and will eventually help fix several outstanding TODOs.

This mirrors incubator changes from llvm/clangir#1593
@xlauko xlauko mentioned this pull request May 1, 2025
Merged
xlauko added a commit to llvm/llvm-project that referenced this pull request May 2, 2025
@xlauko
[CIR] Refactor IntType constraints (#138106)
3f367a6 
- Adds `CIR_` prefixes to integer type constraints types to disambiguate their names from other dialects.
- Renames `PrimitiveInt` to `CIR_AnyFundamentalIntType` to align more with constrian conventions.
- Adds bunch of helper constraint classes to be able to define base types to reduce clutter of necessary type casts.
- Reworks constraints to use `CIR_ConfinedType` to avoid repeating validation checks.
- Adds `IntOfWidths` variadic bitwidth constraint to reduce boilerplate code needed to handle multi-bitwidth parameters.
- Constraints are moved into a separate file, which starts decoupling of constraints and types to remove the cyclic dependency between types and attributes and will eventually help fix several outstanding TODOs.

This mirrors incubator changes from llvm/clangir#1593
IanWood1 pushed a commit to IanWood1/llvm-project that referenced this pull request May 6, 2025
@xlauko @IanWood1
[CIR] Refactor IntType constraints (llvm#138106)
09ad73c 
- Adds `CIR_` prefixes to integer type constraints types to disambiguate their names from other dialects.
- Renames `PrimitiveInt` to `CIR_AnyFundamentalIntType` to align more with constrian conventions.
- Adds bunch of helper constraint classes to be able to define base types to reduce clutter of necessary type casts.
- Reworks constraints to use `CIR_ConfinedType` to avoid repeating validation checks.
- Adds `IntOfWidths` variadic bitwidth constraint to reduce boilerplate code needed to handle multi-bitwidth parameters.
- Constraints are moved into a separate file, which starts decoupling of constraints and types to remove the cyclic dependency between types and attributes and will eventually help fix several outstanding TODOs.

This mirrors incubator changes from llvm/clangir#1593
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request May 6, 2025
@xlauko @github-actions
Automerge: [CIR] Refactor IntType constraints (#138106)
6be1cb8 
- Adds `CIR_` prefixes to integer type constraints types to disambiguate their names from other dialects.
- Renames `PrimitiveInt` to `CIR_AnyFundamentalIntType` to align more with constrian conventions.
- Adds bunch of helper constraint classes to be able to define base types to reduce clutter of necessary type casts.
- Reworks constraints to use `CIR_ConfinedType` to avoid repeating validation checks.
- Adds `IntOfWidths` variadic bitwidth constraint to reduce boilerplate code needed to handle multi-bitwidth parameters.
- Constraints are moved into a separate file, which starts decoupling of constraints and types to remove the cyclic dependency between types and attributes and will eventually help fix several outstanding TODOs.

This mirrors incubator changes from llvm/clangir#1593
GeorgeARM pushed a commit to GeorgeARM/llvm-project that referenced this pull request May 7, 2025
@xlauko @GeorgeARM
[CIR] Refactor IntType constraints (llvm#138106)
592bcbc 
- Adds `CIR_` prefixes to integer type constraints types to disambiguate their names from other dialects.
- Renames `PrimitiveInt` to `CIR_AnyFundamentalIntType` to align more with constrian conventions.
- Adds bunch of helper constraint classes to be able to define base types to reduce clutter of necessary type casts.
- Reworks constraints to use `CIR_ConfinedType` to avoid repeating validation checks.
- Adds `IntOfWidths` variadic bitwidth constraint to reduce boilerplate code needed to handle multi-bitwidth parameters.
- Constraints are moved into a separate file, which starts decoupling of constraints and types to remove the cyclic dependency between types and attributes and will eventually help fix several outstanding TODOs.

This mirrors incubator changes from llvm/clangir#1593
@xlauko xlauko deleted the users/xlauko/cir-type-constraints branch June 5, 2025 20:09
terapines-osc-cir pushed a commit to Terapines/clangir that referenced this pull request Sep 2, 2025
@xlauko @lanza
[CIR] Refactor IntType constraints (llvm#1593)
ff0a5b2 
- Adds `CIR_` prefixes to integer type constraints types to disambiguate their names from other dialects.
- Renames `PrimitiveInt` to `CIR_AnyFundamentalIntType` to align more with constraint conventions.
- Adds bunch of helper constraint classes to be able to define base types to reduce clutter of necessary type casts.
- Reworks constraints to use `CIR_ConfinedType` to avoid repeating validation checks. 
- Adds `IntOfWidths` variadic bitwidth constraint to reduce boilerplate code needed to handle multi-bitwidth parameters. 
- Constraints are moved into a separate file, which starts decoupling of constraints and types to remove the cyclic dependency between types and attributes and will eventually help fix several outstanding TODOs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bcardosolopes bcardosolopes bcardosolopes approved these changes

@lanza lanza Awaiting requested review from lanza lanza is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@xlauko @bcardosolopes