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
[LoopVectorize] Vectorize select-cmp reduction pattern for increasing integer induction variable #67812
[LoopVectorize] Vectorize select-cmp reduction pattern for increasing integer induction variable #67812
Changes from 1 commit
6bdc81f
bcab2a6
dfa355b
734da15
2c896c4
1946c8c
e8d5b1d
d2bfe2f
4a88b44
d21b127
2fe0a94
7ebc7d8
3e2e9f1
11900cd
2924bf9
dd21cd3
76e91cc
4f743ab
556743a
a852eb6
4947e0f
6e739d8
7d2a8ec
e530a3a
e50439e
ab0b4d3
97e0433
69cd172
5ebec5d
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.
Do we not also need one for FFindLastIV?
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.
Good question. We don't need to call
AddReductionVar
again for FFindLastIV.This is because at the end of function
isFindLastIVPattern
, IFindLastIV can be transformed into FFindLastIV if the predicate is an fcmp instruction.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 AnyOf recurrence kinds do the same thing, although there is a separate check for
FAnyOf
below which wouldn't be reached ifIAnyOf
matched it first, and is just redundant work if there wasn't a match.We would only see that when looking at iv-descriptors debug output, and there's no tests for that. This code at least reports the correct type for an fcmp.
I figure it's fine with just the single case 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.
It looks like #118393 is attempting to improve 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.
It might be a good idea to indicate whether we've matched IFindLastIV or FFindLastIV because
FindLastIV
doesn't tell you which one.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.
9a0bf28
Sure, I agree that explicitly indicating IFindLast or FFindLast is better.
The new modification directly determines whether to print IFindLast or FFindLast based on the final result. This way, there is no need to call
AddReductionVar
twice, and the correct result can be indicated.If you feel that detecting IFindLast and FFindLast separately would be clearer, please let me know.
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.
needs test without forced interleave count ?
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 want to confirm whether you're trying to test this message?
or if you simply want to see test results without
-force-vector-interleave
?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.
Both
@select_icmp_const_1
and@select_icmp_const_2
look similar to testselect_icmp_nuw_nsw
in Transforms/LoopVectorize/iv-select-cmp-no-wrap.ll.Also, I see the only difference between
@select_icmp_const_1
and@select_icmp_const_2
is the operands to the select are swapped. I'm not sure having both versions really adds much value. Perhaps you can remove both of them and leave the one in iv-select-cmp-no-wrap.llThere 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.
They are indeed quite similar. iv-select-cmp.ll is for testing IR generation, including the case of UF > 1. iv-select-cmp-no-wrap.ll is for testing whether vectorization is legal.
My suggestion is to remove
select_icmp_nuw_nsw
from iv-select-cmp-no-wrap.ll, and retain@select_icmp_const_1
and@select_icmp_const_2
.What do you think?
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.
@david-arm Ping.
Regarding my test file proposal, what do you think?
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 just delete this file, since it looks practically the same as llvm/test/Transforms/LoopVectorize/iv-select-cmp-no-wrap.ll and doesn't seem to offer any extra value?
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.
No, we must retain
llvm/test/Transforms/LoopVectorize/select-min-index.ll
.select-min-index.ll
demonstrates another semantics: minmax with index. This semantics is similar to FindLastIV but different.BTW, this patch is separated from the minmax with index patch.