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

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6202
 
+  if (!EnableCondStoresVectorization && NumPredStores) {
+    ORE->emit(createMissedAnalysis("ConditionalStore")
----------------
hsaito wrote:
> 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. 
Can you measure any difference? Like, running with and without on the test-suite to see if there are any hits that the new cost model is not picking it up?


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6586
 
+bool LoopVectorizationCostModel::useEmulatedMaskMemRefHack(Instruction *I){
+  // TODO: Cost model for emulated masked load/store is completely
----------------
hsaito wrote:
> 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.
The way we've done in the past was to add the function we expect in TTI and implement it in the base class with a big FIXME, explaining the problem. In this way, targets can "FIXIT" in different ways at different times.

But I'm not against the idea of this hack, if that's the consensus (just had't followed the original thread :).


Repository:
  rL LLVM

https://reviews.llvm.org/D43208