[PATCH] D67764: [LV] Forced vectorization with runtime checks and OptForSize

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

So starting from D65197 <https://reviews.llvm.org/D65197> or so, a loop that is forced to vectorize in a function that is OptForSize, is expected to get vectorized even if it involves replicating the loop.
Another assert which presumably requires similar update is the "Cannot SCEV check stride or overflow when optimizing for size" one.



================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:315
 
+  const LoopVectorizeHints *getHints() { return Hints; }
+
----------------
Method itself should be const. 
Could alternatively get Hints from CostModel.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:2738
+         Legal->getHints()->getForce() != LoopVectorizeHints::FK_Enabled &&
          "Cannot emit memory checks when optimizing for size");
 
----------------
Assert message should say it all, i.e., "... when optimizing for size unless forced to vectorize"; above documentation seems redundant.

In case we are forced to vectorize, optimize for size, and end up replicating the loop due to runtime checks, we should issue an LLVM_DEBUG/ORE message, informing the programmer code bloat may be reduced by removing the forced vectorization, or perhaps by modifying the code s.t. runtime checks are not needed (e.g., adding "restrict").


================
Comment at: llvm/test/Transforms/LoopVectorize/runtime-check-optsize.ll:1
+; RUN: opt < %s -O1 -disable-basicaa -S -o - | FileCheck %s
+
----------------
Suffice to run loop-vectorize only, instead of -O1?

Would be good to add this test to a relevant file, instead of dedicating a new file for each test.


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

https://reviews.llvm.org/D67764