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
[LLVM] Optimize G_SHUFFLE_VECTOR into more efficient generic opcodes #41
base: aie-public
Are you sure you want to change the base?
[LLVM] Optimize G_SHUFFLE_VECTOR into more efficient generic opcodes #41
Changes from all commits
5a53fd6
432c409
0cd7ef6
b5e22cc
d3f07f5
8d757e7
e4b0f01
38729a7
ebe6489
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.
G_UNMERGE_VALUE
isn't really meant to be used for vectors like this, it is actually defined as a scalar opcode. There is a nice drop-in replacement opcode calledG_EXTRACT_SUBVECTOR
which has landed in our tree in early August, but there is no support in other backends for this yet. So this implementation is a necessary evil until the other opcode is able to replace it.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.
Curious: Maybe
IRTranslator
already has a similar piece of code that we can re-use?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.
Not from what I can see,
G_UNMERGE_VALUES
isn't meant for vectors to begin with. It only supports scalars, according to the documentation. Most backends just support this as an extension since there is no other opcode to do it. In theory, this will be ripped out in favour of a simpleG_SUBREGISTER_EXTRACT
in the near future if that opcode gets proper support.Utils.cpp
does have something that is a bit similar, calledllvm::extractParts
, but that splits into two parts while this code goes through it like a binary tree.In any case, I would keep it like this since this code should be replaced before upstreaming and that is easier if we are the only ones depending on it.
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.
Yeah, this is a bit weird. There is an ARM64 test which creates an array of 8 1-bit numbers that the legalizer turns into 8-bit numbers. At the moment, their legalizer doesn't do this for some of the replacement op codes.
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.
Maybe do these tests in a callback? Now, if one fails, it will exit the combiner completely rather than continuing on. For now, this is fine, but with more patterns that are close, this might be problems.