[PATCH] D27518: Moving isComplex decision to TTI

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:606
+  /// of address-computation cost 
+  struct AddressAccessInfo {
+    bool  isStrided;        /// True in case the access is strided (AddRec).
----------------
magabari wrote:
> 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
I agree a Value* is not appropriate.
I can live with either a struct or SCEV.


================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:921
 
+  bool isExpensiveAddressComputation(TTI::AddressAccessInfo *Info) {
+    int MaxMergeDistance = 64;
----------------
Assuming this stays a struct - could you change all the "Info" variables and arguments to "AddressInfo" or something else that's meaningful?


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1483
+  // compile time, the address computation will not incur more than one extra
+  // ADD instruction (which for now we ignore). 
+  if (Info && !Info->isStrided)
----------------
Maybe it would be better to have the options "0, 1, 10" instead of "0, 10, where the 0 case can hide an add that we ignore"?
I'm saying "maybe" because, as imprecise as this already is, fixing that may just be noise. On the other hand, I'm loathe to leave it as a TODO, because I think that'd require another API change.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6782
+              const Loop *TheLoop) {
+  TargetTransformInfo::AddressAccessInfo Info;            
+  Info.isStrided = false;                                              
----------------
magabari wrote:
> 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
Please don't leave undefined fields.


https://reviews.llvm.org/D27518