[PATCH] D32422: LoopVectorizer: let target prefer scalar addressing computations (+ minor improvements in SystemZTTI)

[Elena Demikhovsky via Phabricator via llvm-commits]

delena added a comment.

The code LGTM, I do not have more comments on it.  I can't say anything about profitability of this algorithm for SystemZ, may be you need additional LGTM from one of SystemZ developers?



================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7308
+      if (PtrDef && TheLoop->contains(PtrDef) &&
+          getWideningDecision(&I, VF) != CM_GatherScatter)
+        AddrDefs.insert(PtrDef);
----------------
jonpa wrote:
> delena wrote:
> > At this point you may have "interleave" decision. I assume, you have nothing to do with it.
> > I'd check getWideningDecision(&I, VF) == CM_Scalarize)
> As explained before, there are just a few loops that this relates to. When I tried to handle only CM_Scalarize like you suggested, it changed just 17 loops, but it seemed to be just a bit better to do the interleaved accessess as well. 
> 
> I believe in this case the the address is scalar, but that register is also used for something else. That means that it ends up in a vector register and has to be extracted.
> 
> The reason I would also like to see this is as explained before that due to LSR, it is generally better if all else same to keep addressing scalar.
> 
Ok. You scalarize address calculation even for wide loads. Probably it make sense for SystemZ. 


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7321
+        if ((InstOp->getParent() == I->getParent()) && !isa<PHINode>(InstOp) &&
+            AddrDefs.insert(InstOp).second == true)
+          Worklist.push_back(InstOp);
----------------
jonpa wrote:
> delena wrote:
> > AddrDefs.insert(InstOp).second == true)  -> AddrDefs.count(InstOp)
> no - I want to insert InstOp into AddrDefs if it isn't there, right?
ok, you are right!


https://reviews.llvm.org/D32422