[PATCH] D27518: Moving isComplex decision to TTI

[Mohammed Agabaria via Phabricator via llvm-commits]

magabari added a comment.

fixed issues pointed by Michael



================
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:
> 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.
SCEV can't replace the struct, since you still need more info.
nullptr -> means default behavior
struct->isStrided = false -> means after check it is not strided at all
struct->isStrided = true -> you can look on the stride info or SCEV
but SCEV can be part of the struct if needed.
So in case there is no objections I prefer to go with struct which will contain bool and SCEV.


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


================
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)
----------------
mkuper wrote:
> 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.
fixed


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6782
+              const Loop *TheLoop) {
+  TargetTransformInfo::AddressAccessInfo Info;            
+  Info.isStrided = false;                                              
----------------
mkuper wrote:
> 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.
fixed


https://reviews.llvm.org/D27518