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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin 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);
----------------
wmi wrote:
> Perhaps make it a clang option instead of an internal option.
I think that would be more appropriate once the overall approach is more mature.


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


================
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());
----------------
wmi wrote:
> Perhaps extract the code into an insertPrefetch function? This part is machine dependent. 
The whole pass is machine dependent.


================
Comment at: lib/Target/X86/X86TargetMachine.cpp:500-501
   }
+  addPass(createX86DiscriminateMemOpsPass());
+  addPass(createX86InsertPrefetchPass());
 }
----------------
wmi wrote:
> wmi wrote:
> > Does prefetch insertion need to be guarded by "getOptLevel != CodeGenOpt::None"?
> Have you considered to make the two passes architecture independent?
Not really, someone might want no optimization and prefetch insertion (although this is a very odd scenario and a very weak argument). Happy to guard it if there's a reason to.


================
Comment at: lib/Target/X86/X86TargetMachine.cpp:500-501
   }
+  addPass(createX86DiscriminateMemOpsPass());
+  addPass(createX86InsertPrefetchPass());
 }
----------------
mtrofin wrote:
> wmi wrote:
> > wmi wrote:
> > > Does prefetch insertion need to be guarded by "getOptLevel != CodeGenOpt::None"?
> > Have you considered to make the two passes architecture independent?
> Not really, someone might want no optimization and prefetch insertion (although this is a very odd scenario and a very weak argument). Happy to guard it if there's a reason to.
It's too early for generalizing this outside X86.


Repository:
  rL LLVM

https://reviews.llvm.org/D54052