Building on D142885 and D142589, retire the SinkAfter map from the
recurrence handling code. It is replaced by checking whether it is
possible to sink all users of a recurrence directly in VPlan. This
results in simpler code overall and allows to handle additional cases
(see the improvements in @test_crash).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/include/llvm/Analysis/IVDescriptors.h | ||
---|---|---|
189 | Leave a comment that this optimistically assumes instructions may be reordered successfully if needed? | |
llvm/lib/Analysis/IVDescriptors.cpp | ||
967 | (Relying here on Previous is fragile as it may have been considered for sinking.) | |
985 | nit: such a set can be used to store "Seen" recipes avoiding revisiting recipes as they are added to the worklist and then iterating over them in dominance order to sink them. | |
llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp | ||
651 | nit: is this consideration of replicate regions is still needed here, or can be replaced by an assert? | |
689 | Is this change to support multiple def'd recipes independent of the rest of this patch? | |
740 | Is SeenPrevious really needed, to make sure chained FORs are processed in order, or can it be removed - given that every FOR takes care of its own sinkings followed by introducing its splice hooking it up to defs and uses? Potentially sinking candidates multiple times. | |
llvm/lib/Transforms/Vectorize/VPlanTransforms.h | ||
79 | Explain return value? | |
llvm/test/Transforms/LoopVectorize/first-order-recurrence-chains.ll | ||
637 | nit: update name of test, and its comment? |
Address latest comments, thanks!
llvm/include/llvm/Analysis/IVDescriptors.h | ||
---|---|---|
189 | Added, thanks! | |
llvm/lib/Analysis/IVDescriptors.cpp | ||
967 | Agreed, but I think until we fully check legality in VPlan I think it is needed for now here (and I think it is not different to the current handling) | |
llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp | ||
651 | Thanks, adjusted in c18bc7f7feac. | |
689 | Yep that's independent of the patch, I restored the original code here. | |
740 | I checked and it is not needed in the latest version indeed! It also yields a nice improvement in @test_pr54223_sink_after_insertion_order! Updated to remove it. | |
llvm/lib/Transforms/Vectorize/VPlanTransforms.h | ||
79 | Added a comment, thanks! | |
llvm/test/Transforms/LoopVectorize/first-order-recurrence-chains.ll | ||
637 | Added, thanks! |
LGTM
llvm/lib/Analysis/IVDescriptors.cpp | ||
---|---|---|
967 | (Above comment refers to ignoring if SinkAfter(Previous), which is the source of fragility/optimism.) | |
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp | ||
8997 | TODO (Independent of this patch): Consider moving this potentially bailing-out check earlier to save compile-time for bailed-out cases, which would avoid vectorizing altogether. | |
llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp | ||
660 | Add meaning of return value? | |
669–672 | "cannot" >> "must not"? | |
740 | Very well! A direct consequence of VPlan's design to rely on updated def-use IR rather than recorded maps and sets. |
Heads up: This change is causing a clang hang when building xz utils (https://github.com/tukaani-project/xz). I'll file a bug and attach a repro asap.
Thanks for the report! I recommitted the change with a fix for the compile-time issue in 6b8d19d2b57ecd45eb2ff1964798f3cd8c907ac4. The new version of the patch adds a set to avoid duplicating work in isFixedOrderRecurrence, which was previously done through the removed SinkAfter map.
llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp | ||
---|---|---|
660 | Thanks, documented! | |
669–672 | Adjusted the comment, thanks! |
Leave a comment that this optimistically assumes instructions may be reordered successfully if needed?