[PATCH] D80870: [LV] Make sure smallest/widest type sizes are powers-of-2.

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5011
 
   unsigned MaxVF = UserVF ? UserVF : computeFeasibleMaxVF(TC);
   assert((UserVF || isPowerOf2_32(MaxVF)) && "MaxVF must be a power of 2");
----------------
Ayal wrote:
> The original motivation to assert that MaxVF is a power of 2, in D80491, was for concluding that any chosen VF will divide a constant trip count.
> Perhaps a better fix than D80491 would be (have been ..., as noted in the summary) to round MaxVF down to a power of 2 here, rather than forcing computeFeasibleMaxVF() and/or getSmallestAndWidestTypes() to return powers of 2.
Rounding down MaxVF returned by computeFeasibleMaxVF (maybe at the end of that function rather than here) would probably work better for our OOT target (with 160-bit register and i40 types, so neither the Smallest/WidestType nor the TTI.getRegisterBitWidth(true) returns a power-of-2 value in that situation).


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5229
 
-  return {MinWidth, MaxWidth};
+  // Round up to the next power of 2, if min or max widths aren't powers of 2.
+  return {PowerOf2Ceil(MinWidth), PowerOf2Ceil(MaxWidth)};
----------------
Ayal wrote:
> The critical part is to make sure VF does not exceed `MaxSafeRegisterWidth`; this indeed holds when rounding `MinWidth` and `MaxWidth` up to powers of 2; might not hold when rounding them down, conceptually.
> 
> But perhaps non-power-of-2 sized types should be skipped when computing MinWidth and MaxWidth, related to the last "also" part of the FIXME above? cf. `hasIrregularType()`. 
I did play around with ignoring "irregular" types here a few days ago. In most situations I think it is ok. But I also noticed that it is possible to for example get <4 x i15> in the vector body, with a loop doing `store i15 7, i15* ptr, align 2`.

So ignoring the type when determining Smallest/Widest type does not completely prevent us from creating vectors with irregular types (as it seems). And that vector could be larger than the max register width. Although, considering that the resulting <4 x i15> vector store in my test will use a packed layout, that case actually looks like a bug (I'll probably end up writing a PR about it).




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80870