[PATCH] D27518: Moving isComplex decision to TTI

[Mohammed Agabaria via Phabricator via llvm-commits]

magabari marked 3 inline comments as done.
magabari added a comment.

fixed some comments. 
In my opinion we should still keep TTI as a lightweight interface which is not depended on SCEV.
So i think that passing struct which can hold all the properties still can be better approach. Even for future enhancements for that function.
please tell me what do you think.



================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:606
+  /// of address-computation cost 
+  struct AddressAccessInfo {
+    bool  isStrided;        /// True in case the access is strided (AddRec).
----------------
mkuper wrote:
> hfinkel wrote:
> > mkuper wrote:
> > > delena wrote:
> > > > mkuper wrote:
> > > > > delena wrote:
> > > > > > Why do you need a struct? "Value *" will contain everything.
> > > > > > 
> > > > > > Value *Stride;
> > > > > > 
> > > > > > if (Stride == nullptr) - no stride,
> > > > > > dyn_cast<ConstantInt>(Stride) - answers the question isConstant()
> > > > > >  
> > > > > IIUC, we don't necessarily have a Value* in hand, this originates from a SCEV, right?
> > > > yes. We can use const SCEV*. 
> > > I'm not sure passing a SCEV* to TTI makes sense, in terms of layering.
> > I don't see why that should be a problem. The backends already depend on SCEV (and the rest of analysis) because of IR-level passes. TTI should definitely use SCEV where appropriate.
> Never mind me, then. SCEV it is.
passing only Value* has some cons and doesn't solve the general problem.
1. it depends on SCEV
2. currently we differentiate between strided and not strided access, but i think that there is more than those 2 cases so i thought creating some struct will be better approach


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6782
+              const Loop *TheLoop) {
+  TargetTransformInfo::AddressAccessInfo Info;            
+  Info.isStrided = false;                                              
----------------
mkuper wrote:
> mkuper wrote:
> > Extra spaces after the line.
> > (On the line below too.)
> You don't seem to initialize the fields.
add initialize for isConstant field too. last field is undefined in case it's not constant stride


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:7046
 
-      // True if the memory instruction's address computation is complex.
-      bool IsComplexComputation =
-          isLikelyComplexAddressComputation(Ptr, Legal, SE, TheLoop);
+      // Get some info about the address access complexity in the loop   
+      TargetTransformInfo::AddressAccessInfo Info = 
----------------
mkuper wrote:
> Please elaborate about what you actually get, rather than just "some info"
> (this is really "Figure out whether the access is strided and what the stride is..." or something like that).

fixed


https://reviews.llvm.org/D27518