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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

In D80870#2066895 <https://reviews.llvm.org/D80870#2066895>, @manojgupta wrote:

> D80491 <https://reviews.llvm.org/D80491> has been breaking our ToT builds for a week now. Can this be submitted soon. If not please consider reverting D80491 <https://reviews.llvm.org/D80491>.


Sorry about this. But note that reverting D80491 <https://reviews.llvm.org/D80491> will only remove an assert, leading to potentially wrong code being generated silently.
Perhaps additional lit tests could be devised to help exercise failing behaviors(?)

In D80870#2067120 <https://reviews.llvm.org/D80870#2067120>, @fhahn wrote:

> Updated to round down in computeFeasibleMaxVF.


Update looks good to me, thanks! Added a few minor comments.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5061
 
   // Ensure MaxVF is a power of 2; the dependence distance bound may not be.
   WidestRegister = PowerOf2Floor(WidestRegister);
----------------
Suggest to add to the comment that type sizes may also not be powers of 2.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5062
   // Ensure MaxVF is a power of 2; the dependence distance bound may not be.
   WidestRegister = PowerOf2Floor(WidestRegister);
 
----------------
Rounding WidestRegister down to a power of 2 is no longer needed; rounding-down MaxVectorSize alone suffices to ensure the method returns a power of 2.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5122
   }
-  return MaxVF;
+  return PowerOf2Floor(MaxVF);
 }
----------------
This should be redundant; i.e., at this point MaxVF can be asserted to be a power of 2. Only way it might not be is because of getMinimumVF(), which, being a minimum, should probably not be rounded down silently (see above comment).


================
Comment at: llvm/test/Transforms/LoopVectorize/X86/fp80-widest-type.ll:7
+
+; Make sure non-power-of-2 types are round up to the next power of 2.
+
----------------
Comment should be updated. E.g., ";Make sure non-power-of-2 types are handled correctly, i.e., MaxVF is still a power-of-2."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80870