[PATCH] D89566: [LV] Epilogue Vectorization with Optimal Control Flow

Tue Dec 1 09:11:33 PST 2020

fhahn added a comment.

In D89566#2419093 <https://reviews.llvm.org/D89566#2419093>, @bmahjour wrote:

> Thank you for your reviews @SjoerdMeijer @fhahn .
>
> I totally see the benefit of D92129 <https://reviews.llvm.org/D92129> as you illustrated in D92132 <https://reviews.llvm.org/D92132> and I think it can be used to replace things like `fixupIVUsers()` which would also make it easier to extend epilogue vectorization. As for broader subclassing, it shouldn't prevent us from extending it in a way that uses VPlan. If specialization at the subclass level is not necessary (ie there is code reuse opportunity) then we should still implement it in the superclasses and inherit it in the subclasses. If specialization is not applicable to any other part of the transform then it will happen at the subclass level where it belongs, //whether using VPlan or not//.
>
> Having said that there is a timing aspect. I see your point that if we tried to extend epilogue vectorization now via specialization of parts of the codegen, //that don't use VPlan currently//, then we'd have to change more places in the future. From that perspective a more targeted specialization would help by making it harder to extend epilogue vectorization without having improved VPlan. On the other hand, some improvements to the epilogue vectorization would become contingent on improving VPlan (which is not an issue for me, but others may disagree).
>
> Having a skeleton creator class would require some refactoring of `InnerLoopVectorizer` and `LoopVectorizationPlanner`, so it would warrant having a separate patch for it. I can do it after this one lands.

OK great. It sounds like there's agreement on the further direction overall and we can work on that in-tree. I'll work on getting the pieces ready to handle live-ins/live-outs in VPlan as required. As for the sekeleton creator, I might be able to take a look over the next week.

LGTM, thanks (meant to respond a bit earlier today :)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89566/new/

https://reviews.llvm.org/D89566