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.
[CLANG][AArch64]Add Neon vectors for mfloat8_t #99865
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
[CLANG][AArch64]Add Neon vectors for mfloat8_t #99865
Changes from all commits
2197d41
a09a8e1
a3d988a
a605455
4c370d5
2384c5d
f304872
c9737d6
0074b75
b65c5bf
9c186c8
ac5c0dc
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.
Is there a reason for the new types to be guarded? When looking at
arm_vector_types.h
the guards are only used forfloat64
based types, with the other vector types unguarded.Also, and this might necessitate reintroducing some of the code you were asked to remove, I'd rather
emitNeonTypeDefs
have the capability to emit the typedefs because that'll verify we have the necessaryTypeSpec
plumbing required to add the builtins.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.
That makes sense. I was keen to remove the
TypeSpec
stuff as it was not used at all, but these typedefs should probably be emitted byemitNeonTypeDefs
as they are.. well, still Neon types after all.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.
So if we dont want to protect that to 64 bits architecture, then I have to change Sema and ASTContext to introduce the neon type for other architectures that are not 64 bits. Otherwise they fail, because the typedef and the builtin are only introduced/added when Target.hasAArch64SVETypes.
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.
Oh, I hadn't realised this was a shared header with
arm
as well asaarch64
. Fair enough, I retract my comment.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 it still possible to make use of
emitNeonTypeDefs
? I can see that also emits the inclusion guards when necessary.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.
Based on an offline conversation it has been agreed to wait for the scalar type to land before trying to extend
emitNeonTypeDefs
and thus this PR can continue as is..