Regarding this NFC change, please refer to the discussion in this thread. https://reviews.llvm.org/D150851#4467261
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Time | Test | |
---|---|---|
1,910 ms | x64 windows > lld.ELF::export-dynamic-symbol.s |
Event Timeline
Thanks for following-up on this! Adding various minors nits.
llvm/include/llvm/Analysis/IVDescriptors.h | ||
---|---|---|
44 | ||
52 | nit: while we're here, worth clarifying that FMulAdd is essentially an FAdd reduction whose reduced values are products fused with the add in fmuladd, as in a dot-product sum(a[i]*b[i]). | |
53–54 | ||
237 | Would it make (more) sense to rename this isAnyOfRecurrenceKind()? | |
llvm/include/llvm/Transforms/Utils/LoopUtils.h | ||
402 | createAnyOfTargetReduction()? | |
llvm/lib/Target/AArch64/AArch64TargetTransformInfo.cpp | ||
3267 | nit: while we're here, worth placing these last - after FMulAdd, as they appear in RecurKind definition? | |
llvm/lib/Target/RISCV/RISCVTargetTransformInfo.h | ||
292 | ditto | |
llvm/test/Transforms/LoopVectorize/smax-idx.ll | ||
58 | nit: MMI - Min-or-Max-with-Index? (nit: if index is not used outside the loop, best eliminate its computation as dead code.) | |
62 | Indeed not an any_of reduction, but an SMax one. | |
91 | Indeed not an any_of reduction, but an SMax one. |
llvm/include/llvm/Analysis/IVDescriptors.h | ||
---|---|---|
44 | LG, this change will push directly to llvm-project repo. | |
52 | LG, this change will push directly to llvm-project repo. | |
53–54 | Since currently we only have implemented the integer version of any_of, would it be better to retain it specifically for integers? | |
237 | I suggest this: static bool isAnyOfRecurrenceKind(RecurKind Kind) { return Kind == RecurKind::IAnyOf || Kind == RecurKind::FAnyOf; } static bool isSelectCmpRecurrenceKind(RecurKind Kind) { return isAnyOfRecurrenceKind(Kind); } | |
llvm/test/Transforms/LoopVectorize/smax-idx.ll | ||
58 |
Yes, it's min/max with index. I think I should put the full name before the short name.
This case checks if the maximum result is not used outside the loop. Regarding the index, if I recall correctly, the vectorizer should be able to perform dead code elimination for values that are not used outside the loop. | |
62 | How do you feel about the following version? ; Check if it is a min/max with index (MMI) pattern when the ; min/max value is not used outside the loop. ; ; Currently, the vectorizer checks if smax value is used outside ; the loop. However, even if only the index part has external users, ; and smax itself does not have external users, it can still form a ; MMI pattern. | |
91 | Currently, all the reductions of the form select(cmp()) do not support cmp with more than one user, including min/max reductions. |
llvm/include/llvm/Analysis/IVDescriptors.h | ||
---|---|---|
53–54 |
Ah, good clarification, worth a TODO somewhere, possibly attached to a negative test: this pattern is easily applicable to x,y of any type. How about: IAnyOf, ///< any_of reduction with select(icmp(),t,f) and invariant t or f (both integers) IAnyOf ///< any_of reduction with select(fcmp(),t,f) and invariant t or f (both integers) AnyOf is essentially a boolean OR reduction, so should apply to both integer and floating point return values. The predicate producing the reduced values can be provided via icmp or fcmp. The end boolean result could be converted to a {true-value, false value} pair of any type, preferably after the loop. The types of true-value and false-value must match each other, but are independent of the type of comparison. | |
237 | Will isSelectCmpRecurrenceKind() still be needed? | |
llvm/lib/Analysis/IVDescriptors.cpp | ||
806–807 | ? | |
llvm/lib/Transforms/Utils/LoopUtils.cpp | ||
1124 ↗ | (On Diff #543894) | ? |
llvm/test/Transforms/LoopVectorize/smax-idx.ll | ||
58 | Ah, right, thanks for clarifying. | |
62 |
LGTM. Just noting that in some cases it suffices to report the index alone, e.g., when the smax value can be easily retrieved/recomputed using its index. | |
91 |
If min/max is computed with a cmp/select, this cmp will be reused to select its index. By Combination pass you mean instcombine? Anyhow, worth prefixing with TODO. | |
91 | (All reductions require all operations along their chain to have a single user, except for a single live-out. But this test focuses on MMI, which would need to handle cmp feeding two selects as part of its chain.) |
llvm/include/llvm/Analysis/IVDescriptors.h | ||
---|---|---|
237 | Because eventually, D150851 will need isSelectCmpRecurrenceKind(). Both AnyOf and FindLastIV share common characteristics, such as similar identification methods. Using isSelectCmpRecurrenceKind() to control similar behavior should be better than isAnyOfRecurrenceKind() || isFindLastIVRecurrenceKind(). However, I also agree that isSelectCmpRecurrenceKind() might lead to confusion since the format of min max reduction is really similar. |
llvm/include/llvm/Analysis/IVDescriptors.h | ||
---|---|---|
44 | ||
52 | ||
237 |
| |
llvm/lib/Analysis/IVDescriptors.cpp | ||
806–807 | I have not renamed isSelectCmpPattern() to isAnyOfPattern() yet. The reasons for this decision are explained in previous comment https://reviews.llvm.org/D155786/new/#inline-1515374. | |
llvm/test/Transforms/LoopVectorize/smax-idx.ll | ||
91 | Yes, it is instcombine pass. |
llvm/include/llvm/Analysis/IVDescriptors.h | ||
---|---|---|
53–54 | Re-write comments and added TODO. |
llvm/test/Transforms/LoopVectorize/smax-idx.ll | ||
---|---|---|
95 | Typo: duplicated. |
llvm/test/Transforms/LoopVectorize/smax-idx.ll | ||
---|---|---|
95 | Nice catch. |
llvm/include/llvm/Analysis/IVDescriptors.h | ||
---|---|---|
53 | ||
55 | ||
57–58 | Should in general apply to any type, including pointer etc. | |
237 | Then let D150851 argue in favor of isSelectCmpRecurrenceKind() when it needs it, or be content with isAnyOfRecurrenceKind() || isFindLastIVRecurrenceKind() (which may be fine if used seldom). Here this appears redundant and useless? | |
llvm/lib/Analysis/IVDescriptors.cpp | ||
806–807 | Can this patch simply rename the existing code to read if (isAnyOfRecurrenceKind(Kind)) return isAnyOfPattern(L, OrigPhi, I, Prev); leaving future support of FindLastIV in D150851 to add what it needs, e.g., possibly adding if (isFindLastIVRecurrenceKind(Kind)) return isFindLastIVPattern(L, OrigPhi, I, Prev); ? | |
llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp | ||
1273 ↗ | (On Diff #545906) | (While we're here.) |
llvm/test/Transforms/LoopVectorize/smax-idx.ll | ||
89 |
llvm/include/llvm/Analysis/IVDescriptors.h | ||
---|---|---|
57–58 | Indeed! | |
237 | Of course, we can remove isSelectCmpRecurrenceKind() in this patch. After rebasing D150851, I'll temporarily provide a restored implementation of isSelectCmpRecurrenceKind(). Then, we can discuss whether to keep it, rename it, or directly use isAnyOfRecurrenceKind() || isFindLastIVRecurrenceKind(). | |
llvm/lib/Analysis/IVDescriptors.cpp | ||
806–807 | Sure, we can discuss later. |
This looks good to me, thanks for the cleanup!
Adding a couple of minor nits.
llvm/include/llvm/Transforms/Utils/LoopUtils.h | ||
---|---|---|
375–378 | Fixing this original documentation, while we're updating it: there is no \p LoopExitInst here. | |
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp | ||
4029–4032 ↗ | (On Diff #546382) |
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp | ||
---|---|---|
4029–4032 ↗ | (On Diff #546382) |