[PATCH] D155960: [NaryReassociate] Use new access type aware getGEPCost

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/lib/Analysis/TargetTransformInfo.cpp:243
+    AccessType = GEP->user_back()->getAccessType();
+  }
+  SmallVector<const Value *> Ops(GEP->indices());
----------------
luke wrote:
> ebrevnov wrote:
> > I don't think it's a good idea to have one more place where we define default value for AccessType. Especially taking into account it's incompatible with existing ones. The problem I see is that AccessType will be defaulted to different values depending on particular API used. It's unexpected to get different results depending if getInstructionCost or getGEPCost is used.
> > 
> > I believe we should leave a single place (inside TargetTransformInfoImplCRTPBase::getGEPCost) where default is selected.
> Unfortunately we lose access to the GEP's users in TargetTransformInfoImplCRTPBase::getGEPCost, since it works on operands and not the GEP itself, so I'm not sure if we can calculate the default there.
> 
> > It's unexpected to get different results depending if getInstructionCost or getGEPCost is used.
> getInstructionCost actually duplicates this "check first user" logic, but I agree, it's annoying to have this code duplication. IIRC I ended up duplicating it because there was no easy way to call TargetTransformInfo::getGEPCost from inside TargetTransformInfoImplCRTPBase. 
> 
> Maybe we could rejig this helper function to something like `getGEPUserAccessType`, which would then be used like `getGEPCost(GEP->getSourceElementType(), GEP->getPointerOperand(), Ops, getGEPUserAccessType(GEP), CostKind)`; 
> Unfortunately we lose access to the GEP's users in TargetTransformInfoImplCRTPBase::getGEPCost, since it works on operands and not the GEP itself, so I'm not sure if we can calculate the default there.
Ok, I see.

> getInstructionCost actually duplicates this "check first user" logic, but I agree, it's annoying to have this code duplication. 
It's not quite true. getInstructionCost will use default if there is a single user while getGEPCost if there is at least one user.

> Maybe we could rejig this helper function to something like `getGEPUserAccessType`, which would then be used like `getGEPCost(GEP->getSourceElementType(), GEP->getPointerOperand(), Ops, getGEPUserAccessType(GEP), CostKind)`; 

Thinking out it a bit more I find the idea of choosing AccessType implicitly inside TTI problematic. Even if the GEP itself is materialized  it's really implementation specific if its users have been already updated or not. For that reason I think the best would be to take AccessType into account only if it is given and don't guess.

I understand having getGEPCost which takes GEP is convenient but extending TTI's API with new interfaces  with complex interdependencies is error prune  (due to type erased nature of TTI) . Probably the easiest solution would be to simply pass all args each time (as it's done now) or introduce say getFullyMaterializedGEPCost utility somewhere else (not inside TTI).




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155960/new/

https://reviews.llvm.org/D155960