[PATCH] D43208: [LV][nearly NFC] Move isLegalMasked* functions from Legality to CostModel

[Hideki Saito via Phabricator via llvm-commits]

hsaito added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6202
 
+  if (!EnableCondStoresVectorization && NumPredStores) {
+    ORE->emit(createMissedAnalysis("ConditionalStore")
----------------
rengolin wrote:
> hsaito wrote:
> > Do we even want to keep this bail out? 
> I think this is done more as diagnosis than bail out.
In my opinion, this is a bail out in the sense that we blindly set VF=1 and say "go scalar". What's special about conditional stores compared to other "less than optimal" vector operations? We don't have such "enable" flags for every single one of them. Do we still consider the code to support conditional stores semi-"experimental"?

I just wanted to point that out. If anyone wants to keep this, I won't insist. 


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6586
 
+bool LoopVectorizationCostModel::useEmulatedMaskMemRefHack(Instruction *I){
+  // TODO: Cost model for emulated masked load/store is completely
----------------
rengolin wrote:
> What if this was a hook into the TTI, it could use the targets' information to get a slightly more accurate cost estimate (ie, how hacky is the emulation, or even if one is available through its back-end)
Thanks. That's actually my preferred solution (http://lists.llvm.org/pipermail/llvm-dev/2018-January/120271.html) and Hal Finkel agrees (http://lists.llvm.org/pipermail/llvm-dev/2018-January/120273.html). If there are others feeling the same way (or opposite way), please add your voice here.

Having said that, is it okay for me to take the TTI approach as a separate patch submission? I'm sure TTI discussion will involve more stakeholders and the bulk of that discussion is tangent from fixing vectorizer's architectural issue related to isLegalMasked* this patch is trying to address.

This is especially so with the hacky NumberOfStoresToPredicate getting exposed outside of LoopVectorize code. I could imagine some reviewers would be asking us vectorizer developers to "tune the cost first so that the hacky NumberOfStoresToPredicate doesn't have to be leaked out of LoopVectorize". I don't think we should wait fixing the architectural problem until tuning is done.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6588
+  // TODO: Cost model for emulated masked load/store is completely
+  // broken. This hack guides the cost model to use an artificially high
+  // high enough value to practically disable vectorization with such
----------------
rengolin wrote:
> high / high
Will fix. Thanks.


Repository:
  rL LLVM

https://reviews.llvm.org/D43208