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

[Luke Lau via Phabricator via llvm-commits]

luke added inline comments.


================
Comment at: llvm/lib/Analysis/TargetTransformInfo.cpp:243
+    AccessType = GEP->user_back()->getAccessType();
+  }
+  SmallVector<const Value *> Ops(GEP->indices());
----------------
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)`; 


================
Comment at: llvm/lib/Transforms/Scalar/NaryReassociate.cpp:331
-  SmallVector<const Value *, 4> Indices(GEP->indices());
-  return TTI->getGEPCost(GEP->getSourceElementType(), GEP->getPointerOperand(),
-                         Indices) == TargetTransformInfo::TCC_Free;
----------------
ebrevnov wrote:
> It seems to be possible to simplify all other existing calls to getGEPCost the same way. Are you going to fix them as well?
Ideally yes, there's at least one other place in NaryReassociate that I'm aware of. I was planning on leaving that to a separate patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155960