[PATCH] D57837: [LV] Prevent interleaving if computeMaxVF returned None.

Wed Feb 6 14:32:26 PST 2019

fhahn marked 2 inline comments as done.
fhahn added a comment.

Thanks for all the comments!

In D57837#1387764 <https://reviews.llvm.org/D57837#1387764>, @hsaito wrote:

> Other than just the function name and the associated comment:
>
> 1. runtime ptr check && hasBranchDivergence                   interleave w/ VF=1 still okay?

Yep I think so.

> 2. For interleaving, the remaining checks other than TC==0/1 (for better diagnostics) and OptForSize appear useless. Even if TC is evenly divisible by VF or canFoldTailByMasking, we still can't interleave under OptForSize.
> 
>   I think we should start thinking in terms of separate "is vectorization feasible" and "is interleaving feasible" (plus possibly "is unrolling feasible"), even if we evaluate those feasibility in one function.

Agreed, having it done implicitly in a function called computeMaxVF does not seem ideal. From the current behavior of computeMaxVF, I think what we actually want to decide whether to disable interleaving up front is just if we optimize for size or not. What do you think?

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7325
+  unsigned IC = 1;
+  unsigned UserIC = 1;
+  if (MaybeVF) {
----------------
Ayal wrote:
> UserIC should always be set to Hints.getInterleave().
Ah I thought you meant that we should ignore UserIC, in case computeMaxVF returned None, similar to the way we ignore UserVF in that case. Did I miss something?

================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7330
+    // Select the interleave count.
+    IC = CM.selectInterleaveCount(OptForSize, VF.Width, VF.Cost);

----------------
rengolin wrote:
> None of the parameters depend on IC and this call will override the value, why initialise with 1 above?
In case MaybeVF is None, we want to avoid interleaving, that is why it is initialized with 1 and only computed when we have a VF.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57837