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

[Mircea Trofin via Phabricator via llvm-commits]

mtrofin marked 2 inline comments as done.
mtrofin added inline comments.


================
Comment at: lib/Target/X86/X86InsertPrefetch.cpp:171-172
+      prefetches.clear();
+      if (!findPrefetchInfo(Samples, *Current, prefetches))
+        continue;
+      assert(!prefetches.empty());
----------------
wmi wrote:
> mtrofin wrote:
> > 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.
> Then is there anyway to check if there is prefetch which cannot find matching memory instruction?
Indeed - that's on my to-do list as a next step. My plan was to surface that as ORE warnings. I'd prefer doing it in a separate CL, if that's OK.


================
Comment at: lib/Target/X86/X86TargetMachine.cpp:500-501
   }
+  addPass(createX86DiscriminateMemOpsPass());
+  addPass(createX86InsertPrefetchPass());
 }
----------------
wmi wrote:
> mtrofin wrote:
> > 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.
> I was thinking those two passes can be architecture independent with only x86 hook available currently. But I don't have strong opinion if you feel refactoring that later will not be more difficult. 
I understand - I'd prefer doing a refactoring when we have at least a second architecture we want to target.


Repository:
  rL LLVM

https://reviews.llvm.org/D54052