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

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: lib/Target/X86/X86InsertPrefetch.cpp:35-38
+static cl::opt<std::string>
+    PrefetchHintsFile("prefetch-hints-file",
+                      cl::desc("Path to the prefetch hints profile."),
+                      cl::Hidden);
----------------
Perhaps make it a clang option instead of an internal option.


================
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 function only borrows the call target format of afdo. It doesn't really return call target map. Can we rename the function to avoid confusion? Maybe also using a new type name for SampleRecord::CallTargetMap. 


================
Comment at: lib/Target/X86/X86InsertPrefetch.cpp:124-125
+  }
+  assert(max_index + 1 >= 0);
+  assert(static_cast<size_t>(max_index + 1) == Prefetches.size());
+  return !Prefetches.empty();
----------------
llvm style requires a message for an assertion: assert(expr && "msg");
The same for other asserts in the patch.


================
Comment at: lib/Target/X86/X86InsertPrefetch.cpp:171-172
+      prefetches.clear();
+      if (!findPrefetchInfo(Samples, *Current, prefetches))
+        continue;
+      assert(!prefetches.empty());
----------------
Do you think it is worthy to issue a warning so people know there is some mismatch here? 


================
Comment at: lib/Target/X86/X86InsertPrefetch.cpp:175-210
+        unsigned PFetchInstrID = PrefInfo.InstructionID;
+        int64_t Delta = PrefInfo.Delta;
+        const MCInstrDesc &Desc = TII->get(PFetchInstrID);
+        auto *PFetch =
+            MF.CreateMachineInstr(Desc, Current->getDebugLoc(), true);
+        MachineInstrBuilder MIB(MF, PFetch);
+        auto Bias = X86II::getOperandBias(Current->getDesc());
----------------
Perhaps extract the code into an insertPrefetch function? This part is machine dependent. 


================
Comment at: lib/Target/X86/X86TargetMachine.cpp:500-501
   }
+  addPass(createX86DiscriminateMemOpsPass());
+  addPass(createX86InsertPrefetchPass());
 }
----------------
Does prefetch insertion need to be guarded by "getOptLevel != CodeGenOpt::None"?


================
Comment at: lib/Target/X86/X86TargetMachine.cpp:500-501
   }
+  addPass(createX86DiscriminateMemOpsPass());
+  addPass(createX86InsertPrefetchPass());
 }
----------------
wmi wrote:
> Does prefetch insertion need to be guarded by "getOptLevel != CodeGenOpt::None"?
Have you considered to make the two passes architecture independent?


Repository:
  rL LLVM

https://reviews.llvm.org/D54052