-
Notifications
You must be signed in to change notification settings - Fork 182
[CIR] Backport TargetAddressSpaceAttr and Support both existing Lang AS and target AS attributes in pointer types and Global Ops
#1986
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
Open
RiverDave
wants to merge
13
commits into
main
Choose a base branch
from
users/riverdave/backport-target-addrspace
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+462
−321
Open
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
229b5c5
[CIR][AddrSpace] Backport TargetAddressSpaceAttr and Support both lan…
RiverDave c959448
Polish and remove redundant functions
RiverDave a6c0bea
Handle hybrid AS approach with GlobalOps
RiverDave 4a4ea3a
Correct comma on global asm format by providing an optional parser
RiverDave 8aff820
Change Builder for globalOps to take an empty attr to represent defau…
RiverDave 06d4c1b
Remove Target AS handling from `cir::AddressSpace`
RiverDave 49f7c11
Rename AddressSpace to ClangAddressSpace for clarity
RiverDave 5a5d8d2
Fix crash on runnin isa on empty attr
RiverDave 2fe4cfd
Polish comments and docs description
RiverDave 800bc75
Bring Assertion guards for data ptr size based on AS
RiverDave 0ead941
Fix formatting
RiverDave dfd52c2
Fix more formatting errors
RiverDave 0d731a7
rename AS conversion fn for clarity
RiverDave File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a plan to eventually merge these two? I feel it's weird to have two different attributes for it, I wonder if it would be more clean if there's a base
AddressSpaceAttrand these two different ones are just specializations - using CIR_AnyAddressSpaceAttr for this only work for constraints and it would be nice if in general to pass things around in the terms of the generic version. If so, maybe doing it as part of this PR is a good time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not entirely sure if specializations for attributes can be done in MLIR (I might need to research!).
What we could do is to add
MemorySpaceAttrInterfacetoPointerTypeas a paremeter (Similar to how it's done in the ptr dialect:clangir/mlir/include/mlir/Dialect/Ptr/IR/PtrDialect.td
Line 71 in 99388f7
TargetandClangaddress spaces. This last thing I intended to do in a different PR, but I agree It's not entirely correct in its current state.If the above makes sense I'll work on it soon.