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

[Evgeniy via Phabricator via llvm-commits]

ebrevnov added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:302
 
+  // Helper function for using getGEPCost on an already materialized GEP
+  // instruction. Uses the first user as an approximation for AccessType.
----------------
"for using getGEPCost" sounds a bit odd... maybe better say "// Helper function to get cost of already materialized GEP instruction"?


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


================
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;
----------------
It seems to be possible to simplify all other existing calls to getGEPCost the same way. Are you going to fix them as well?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155960