[PATCH] D57382: [LV] Move interleave count computation to LVP::plan().

[Florian Hahn via Phabricator via llvm-commits]

fhahn marked an inline comment as done.
fhahn added a comment.

In D57382#1387143 <https://reviews.llvm.org/D57382#1387143>, @Ayal wrote:

> The bug revealed by the test is rather subtle; does it match the original https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=6477 ?
>
> LV decided (rightfully) not the vectorize the loop in the test having trip-count == 1, but does decide to interleave it (wrongfully, see below). LVP::plan() returns {VF=1,Cost=0} representing the decision not to vectorize, but w/o building any VPlan, which breaks the interleaving decision taken afterwards.
>
> LVP::plan() builds a VPlan for VF=1 only to serve interleaving. So one simple possible fix for this bug is to always build such a VPlan. This patch tries to be smarter, and have LVP::plan() build this single VPlan only if interleaving will actually need it. Another way to building this VPlan only if needed, is to build it on demand, if it's missing, once IC is determined (and before/at setBestPlan() which raised the assert). But note that LVP::plan() refrains from building a VPlan for VF=1 only if MaybeMaxVF is None, meaning "**vectorization** should be avoided **up front**". In such cases, **interleaving** should also be **avoided** up front! This "up frontness" can be propagated from CM.computeMaxVF() to CM.selectInterleaveCount() in one of several ways: raising a "CM.up-front" flag; have LVP::plan() return (1) an Optional<VectorizationFactor>; (2) a std::tie(VF, NoIC), similar to the current patch, but where NoIC is a boolean indicating IC should be set to 1; (3) a {VF=0,Cost=0} to denote that vectorization and interleaving are to be avoided up-front --- note that {VF=1,Cost=0} *also* represents UserVF=1, which *is* subject to interleaving.
>
> It would be good to build VPlans more selectively to save compile-time, and possibly refactor how VF and IC decisions are taken, but better in a separate patch from one that fixes an erroneously missing VPlan?


Thanks Ayal! Do I understand correctly that you propose to first fix the missing VPlan issue by make interleaving respect the fact that interleaving should be avoided when computeMaxVF returns None? If that is the intended behavior, then I think it makes sense to fix that first and I'll prepare a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57382