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.
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
[MLIR][LLVM] Remove typed pointers from the LLVM dialect #71285
[MLIR][LLVM] Remove typed pointers from the LLVM dialect #71285
Changes from 2 commits
de69c84
a1ee18f
95a5519
62cfdef
1ecf826
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why do we need
qualified
here?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.
Without
qualified
, this just prints<1>
, instead of!llvm.ptr<1>
. Tbh, I do not fully understand the logic behind this, but I've observed similar issues in other contexts.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.
@ftynse are you fine with this change or should we get to the bottom of this issue?
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'd like to understand better why and when is this necessary (the feature looks poorly documented), but this should not block this from landing.
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.
This behaviour was introduced by ee09087. As far as I can tell it essentially always omits the dialect prefix if the attribute/type class being parsed is known. Since in this Op it is known to be a
LLVMPointerType
it'll elide the!llvm.ptr
by default when printing.63f0c00 then apparently introduced the
qualified
directive to be able to restore the behaviour to before where the dialect prefix is always printed.Personally I think this is non-intuative and that the default should be printing the full type by default instead, but that is another discussion entirely. Using
qualified
seems to be the intended way to fully qualify the type in the assembly format.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.
This is WAI: qualified is a poor default IMO.
For example why would
!llvm
be a better choice here? And even if it is, why shouldn't we express the choice?Should we even print the type when the address space is the default one? Lot of possible simplifications...
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.
The problem is that not only
!llvm
is omitted, but theptr
prefix as well. It therefore prints just<1>
when using an address space rather than eitherptr<1>
or!llvm.ptr<1>
. This IMO makes it non-obvious what the type is, nor what the meaning of the<1>
is supposed to be.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.
Of course! But the answer is contextual...
For example it's not obvious to me that qualified is just the best answer here, what about the following for example?
I see the design of the assembly format as... a design! It needs to be intentional, and from this point of view we need the most basic blocks to construct a nice format.
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.
The main reason I added
qualified
in this revision is to ensure as few breaking changes as possible, considering that the pointer type changes already forced me to change hundreds of lines.I agree that
qualified
might not be the best idea here, as the "llvm." part brings no benefit. The format you propose would in my opinion be a much better fit. The question remains if we should always print the address space or not, and if we should even print the pointer type if the address space is zero.Note that we should also consider all other operations where I used
qualified
in this discussion, i.e.,LLVM_StoreOp
,LLVM_AtomicRMWOp
,LLVM_AtomicCmpXchgOp
,NVVM_MBarrierArriveSharedOp
.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.
To be clear I wasn't criticizing this PR: I was providing a rational for "building new dialects".