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

dialects: Raise a parsing error in accfg.setup when result type is invalid #3990

Open
wants to merge 1 commit into
base: math-fehr/stack/4
Choose a base branch
from

Conversation

math-fehr
Copy link
Collaborator

@math-fehr math-fehr commented Feb 28, 2025

Stacked PRs:


dialects: Raise a parsing error in accfg.setup when result type is invalid

As there are a redundancy in the assembly format between the first
parsed string and the result type, this should be a parsing error.

…valid

As there are a redundancy in the assembly format between the first
parsed string and the result type, this should be a parsing error.

stack-info: PR: #3990, branch: math-fehr/stack/5
Copy link

codecov bot commented Feb 28, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.19%. Comparing base (04eeaec) to head (1be5202).

Additional details and impacted files
@@                  Coverage Diff                  @@
##           math-fehr/stack/4    #3990      +/-   ##
=====================================================
+ Coverage              90.16%   90.19%   +0.02%     
=====================================================
  Files                    458      458              
  Lines                  58315    58317       +2     
  Branches                5693     5694       +1     
=====================================================
+ Hits                   52581    52600      +19     
+ Misses                  4341     4322      -19     
- Partials                1393     1395       +2     

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

Copy link
Collaborator

@alexarice alexarice left a comment

Choose a reason for hiding this comment

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

Seems reasonable though I'm not really sure why the operation parses/prints a type at this point if it's fully determined by the other inputs.

math-fehr added a commit that referenced this pull request Feb 28, 2025
…valid

As there are a redundancy in the assembly format between the first
parsed string and the result type, this should be a parsing error.

stack-info: PR: #3990, branch: math-fehr/stack/5
Copy link
Collaborator

@JosseVanDelm JosseVanDelm left a comment

Choose a reason for hiding this comment

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

Thanks @math-fehr !

@JosseVanDelm
Copy link
Collaborator

Seems reasonable though I'm not really sure why the operation parses/prints a type at this point if it's fully determined by the other inputs.

@alexarice, are you suggesting that you would expect to print/parse it as:

%state = accfg.setup "acc1" to ("A" = %one : i32)

@alexarice
Copy link
Collaborator

Yes

@alexarice
Copy link
Collaborator

To expand a bit, I wouldn't necessarily expect the custom format to be a certain way (as in I don't feel it's wrong at the moment), but in this case it doesn't feel like the type adds much information to the user, and doesn't seem to be needed by the system.

@JosseVanDelm
Copy link
Collaborator

@alexarice yeah no worries, it's a nice suggestion :) thanks!

// RUN: xdsl-opt --split-input-file --parsing-diagnostics %s | filecheck %s

%one = "test.op"() : () -> i32
%zero = arith.constant 0 : i32
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where is %zero used? Am I missing something?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialects Changes on the dialects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants