[PATCH] D54052: Support for inserting profile-directed cache prefetches

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Target/X86/X86DiscriminateMemOps.cpp:91
+
+  DenseMap<Location, DenseSet<unsigned>> Seen;
+
----------------
Add a comment for this variable.


================
Comment at: lib/Target/X86/X86DiscriminateMemOps.cpp:108
+      }
+      Seen[diToLocation(DI)].insert(DI->getBaseDiscriminator());
+      // Bump the reference DI to avoid cramming discriminators on line 0.
----------------
move the 'insert' before and check the return value (to see if the insertion fails or not). This can save one lookup.


================
Comment at: lib/Target/X86/X86InsertPrefetch.cpp:64
+ErrorOr<SampleRecord::CallTargetMap>
+getCallTargetMap(const FunctionSamples *TopSamples, const MachineInstr &MI) {
+  if (const auto &Loc = MI.getDebugLoc())
----------------
The name of the function can be changed to reflect its real semantics. Also add a comment.


================
Comment at: lib/Target/X86/X86InsertPrefetch.cpp:83
+
+/// Return true if the profided MachineInstruction has cache prefetch hints. In
+/// that case, the prefetch hints are stored, in order, in the Prefetches
----------------
typo: profided.


================
Comment at: lib/Target/X86/X86InsertPrefetch.cpp:101
+  int16_t max_index = -1;
+  for (const auto &S_V : *T) {
+    auto Name = S_V.getKey();
----------------
Add a comment for the loop.


================
Comment at: lib/Target/X86/X86InsertPrefetch.cpp:103
+    auto Name = S_V.getKey();
+    if (Name.consume_front("__prefetch")) {
+      int64_t D = static_cast<int64_t>(S_V.second);
----------------
define a const variable for the prefix string?


Repository:
  rL LLVM

https://reviews.llvm.org/D54052