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

[Renato Golin via Phabricator via llvm-commits]

rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

LGTM, thanks!



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6202
 
+  if (!EnableCondStoresVectorization && NumPredStores) {
+    ORE->emit(createMissedAnalysis("ConditionalStore")
----------------
hsaito wrote:
> rengolin wrote:
> > 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?
> EnableCondStoresVectorization is true by default. So, this IF is no-op unless someone is explicitly setting it to false.
> 
> There are a few LIT tests checking for letting at least one emulated masked store vectorized.
> 
> I saw lots of perf regressions when I ran tests prior to adding cost model hooks (i.e., same cost as the first emulated masked store). On the opposite side of spectrum, if I remember correctly, I didn't see any of our perf tests regressing when I had all emulated masked load/store super high cost ---- that is a very easy way to get the code bit-rottened and thus I won't recommend it. 
I agree, better to drop this check, then.


================
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:
> > 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 :).
> You are the third person saying that the cost hack should be ideally coming from TTI (the other two are Hal and myself). No other opinions explicitly mentioned yet.
> 
> The basic idea of the hack (whether we implement in LoopVectorize or in TTI) is inevitable to make this patch "almost NFC". The TTI version I'm thinking about would have the same basic idea (i.e., return low cost for the first <NumberOfStoresToPredicate> emulated masked stores and super high cost for the rest) ---- because that's the essence of the code prior to this patch.
> 
> For those who are reviewing the TTI version, it would be a lot easier if the patch does not include isLegalMasked* change (from Legality to CostModel). That's another reason why I'd like to do it as a separate patch.
> 
Makes sense.


https://reviews.llvm.org/D43208