[PATCH] D149889: [TTI] Use users of GEP to guess access type in getGEPCost

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:293
+  /// \p AccessTypes is a list of types that might be accessed from memory by
+  /// users of the GEP. getGEPCost will use this determine if the GEP can be
+  /// folded into the addressing mode of a load/store instruction. Note that if
----------------
to determine


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:296
+  /// this is empty, then it assumes that the GEP has no users and will be
+  /// costed as free. If there is a user that isn't a load/store which, then you
+  /// should represent it with a nullptr in the list.
----------------
stray "which"?


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:310
+    // O(N) performance.
+    SmallVector<Type *, 1> AccessTypes;
+    if (!GEP->user_empty())
----------------
ArrayRef should suffice?


================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfoImpl.h:1009
+        }))
+      return !BaseGV ? TTI::TCC_Free : TTI::TCC_Basic;
+
----------------
ABataev wrote:
> return BaseGV ? TTI::TCC_Basic : TTI::TCC_Free;
Why is the BaseGV case not free as well?


================
Comment at: llvm/lib/Transforms/Scalar/LICM.cpp:1346-1354
+    // Account for only the users that are inside CurLoop.
+    SmallVector<Type *> AccessTypes;
+    for (const Value *V : GEP->users()) {
+      auto *I = cast<Instruction>(V);
+      if (CurLoop->contains(I))
+        AccessTypes.push_back(I->getAccessType());
+    }
----------------
luke wrote:
> @nikic I kept the O(N) behaviour here because it looks like we're already iterating through all the users below. But this area seems sensitive. Do we want O(1) behaviour in this block too?
It's okay, but you should drop the extra loop below. It's purpose is to essentially do the check you're doing here, just in a very crude way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149889