[PATCH] D44067: [LV] Improving "Control-Flow-Select" Vectorization remark.

[Vivek Pandya via Phabricator via llvm-commits]

vivekvpandya added a comment.

Few high level comments from my side. For more accurate review I recommend wait until @anemet takes loot at it :)

1. Is there any bug reported for this? if yes then please mention that.
2. I don't understand the purpose of new test case when existing test case already captures this remarks. if any please explain reason to add new test case in description.
3. I hope in addition to LIT based testing you have also done unit testing. You need to enable unit testing with cmake while building LLVM source tree with cmake flag LLVM_BUILD_TESTS=On and then with check-all target in LLVM build system, you can use make check-all or ninja check-all for running those tests.



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:1763
+  /// for 'control flow cannot be substituted for a select' error
+  Instruction *SelectRemarkInstruction;
+
----------------
Instread of Global Valriable I think it is better to use SelectRemarkInstruction as function parameter to blockCanBePredicated() which will be set when it returns false.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5701
       if (auto *C = dyn_cast<Constant>(Operand))
-        if (C->canTrap())
+        if (C->canTrap()){
+          SelectRemarkInstruction = &I;
----------------
Please add space between ) and { so that is consistent with rest of the code.
This applies for all new if statements you added.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:5710
+      if (!LI){
+        SelectRemarkInstruction = LI;
         return false;
----------------
Does using LI as SelectRemarkInstruction provide any more information to optimization remark? if not then I suggest using I. Same thing applies to SI at line no 5732, 5748.


https://reviews.llvm.org/D44067