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: (x86) PR15 - S Operations #2539

Merged
merged 4 commits into from
May 7, 2024
Merged

Conversation

KGrykiel
Copy link
Collaborator

@KGrykiel KGrykiel commented May 5, 2024

Added support for S operations and the unconditional jump. I'll do conditional jumps in a seperate PR as they would involve creating a new register type.

@KGrykiel KGrykiel added the dialects Changes on the dialects label May 5, 2024
@KGrykiel KGrykiel requested review from superlopuh, compor and tarinduj May 5, 2024 13:24
@KGrykiel KGrykiel self-assigned this May 5, 2024
Copy link

codecov bot commented May 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.81%. Comparing base (a131295) to head (135788b).
Report is 7 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main    #2539    +/-   ##
========================================
  Coverage   89.81%   89.81%            
========================================
  Files         351      354     +3     
  Lines       44152    44319   +167     
  Branches     6593     6617    +24     
========================================
+ Hits        39655    39805   +150     
- Misses       3539     3549    +10     
- Partials      958      965     +7     

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

@superlopuh
Copy link
Member

Ok now we're getting to the juicy part of representing assembly control flow in xDSL.

I'm still of the opinion that we should always follow the MLIR invariant that if a region is in SSA, then when a block is entered all the instructions are executed. This jump, for me, should be a terminator, as it is in the riscv_cf dialect. I still think we should remove the non-terminator jumps from the riscv dialect and replace them with the riscv_cf ops instead. The criticism there is that we might not be able to represent dynamic jumping in our IR in that case. The question for me is whether x86 allows dynamic jumps, and whether we want to be able to represent them. A jump to a label can be statically reasoned about, so there we could definitely model it as a terminator op with a successor. Are you planning to add jumps with dynamic offsets?

@KGrykiel
Copy link
Collaborator Author

KGrykiel commented May 5, 2024

I haven't really thought as far as dynamic offsets. My goal for now is to implement a complete basic version that would handle most simple cases and then potentially develop it further based on people's needs. I suppose it's best to keep to static jumps for now?

@superlopuh
Copy link
Member

In that case, I would recommend adding the equivalents to the riscv_cf instructions to x86, with successors and block arguments

@webmiche
Copy link
Collaborator

webmiche commented May 6, 2024

I'm still of the opinion that we should always follow the MLIR invariant that if a region is in SSA, then when a block is entered all the instructions are executed. This jump, for me, should be a terminator, as it is in the riscv_cf dialect. I still think we should remove the non-terminator jumps from the riscv dialect and replace them with the riscv_cf ops instead. The criticism there is that we might not be able to represent dynamic jumping in our IR in that case. The question for me is whether x86 allows dynamic jumps, and whether we want to be able to represent them. A jump to a label can be statically reasoned about, so there we could definitely model it as a terminator op with a successor. Are you planning to add jumps with dynamic offsets?

Big 👍 from me. Note that the MLIR community has been discussing this lately (I remember a talk at eurollvm2023 by Jeff and the mojo people about this). So, let's see what they come up with to do the things they want.

@KGrykiel
Copy link
Collaborator Author

KGrykiel commented May 7, 2024

@superlopuh @webmiche Let me know if this is more or less what you had in mind. I didn't do assembly emission tests yet because those need to supposedly be in a block to work and I don't have that functionality yet. I kind of made a hack to make the other filecheck work in the first place.

Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
@KGrykiel KGrykiel merged commit 92d21ce into main May 7, 2024
10 checks passed
@KGrykiel KGrykiel deleted the KGrykiel/x86dialect-PRs15 branch May 7, 2024 21:40
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
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants