[JIT/llvm-rtdyld] Don't waste cycles invalidating the Instruction cache

[davide]

Summary

We call InvalidateInstructionCache() twice here and I think the second call is just wasted work. Hopefully my guess is correct.

[lhames]

lib/Support/Unix/Memory.inc

266–282 ↗

(On Diff #37048)

Whatever we do here should be consistent for all platforms. I haven't taken a good look at the icache invalidation code - I'll have a think about this tomorrow.

[davide]

Lang, what are your thoughts about this?
Should we invalidate the cache unconditionally instead?

Thanks!

[davide]

Updated after our discussion on IRC. Thanks!

[lhames]

Hi Davide,

I spoke with Jim Grosbach about this and we agreed that 'setExecutable' should do everything required to make the memory executable on the host processor. That means unconditionally setting the permissions and invalidating the icache (on processors that need that). The problem is with clients who call InvalidateInstructionCache after calling setExecutable: that should be redundant. Clients should either call setExecutable alone, or call both protectMappedMemory and InvalidateInstructionCache manually.

So - we should remove the redundant calls, and modify setExecutable where necessary to properly set the permissions.

[lhames]

This comment was removed by lhames.

[lhames]

LGTM, pending the removal of the icache invalidation for the data sections.

tools/llvm-rtdyld/llvm-rtdyld.cpp

432 ↗

(On Diff #38832)

The instruction cache doesn't need to be invalidated for data sections, so this loop should be removed.

llvm/trunk/tools/llvm-rtdyld/llvm-rtdyld.cpp

public:
 
   void *getPointerToNamedFunction(const std::string &Name,
                                   bool AbortOnFailure = true) override {
     return nullptr;
   }
 
   bool finalizeMemory(std::string *ErrMsg) override { return false; }
 
-  // Invalidate instruction cache for sections with execute permissions.
-  // Some platforms with separate data cache and instruction cache require
-  // explicit cache flush, otherwise JIT code manipulations (like resolved
-  // relocations) will get to the data cache but not to the instruction cache.
-  virtual void invalidateInstructionCache();
-
   void addDummySymbol(const std::string &Name, uint64_t Addr) {
     DummyExterns[Name] = Addr;
   }
 
   RuntimeDyld::SymbolInfo findSymbol(const std::string &Name) override {
     auto I = DummyExterns.find(Name);
 
     if (I != DummyExterns.end())
uint8_t *TrivialMemoryManager::allocateDataSection(uintptr_t Size,
   std::string Err;
   sys::MemoryBlock MB = sys::Memory::AllocateRWX(Size, nullptr, &Err);
   if (!MB.base())
     report_fatal_error("MemoryManager allocation failed: " + Err);
   DataMemory.push_back(MB);
   return (uint8_t*)MB.base();
 }
 
-void TrivialMemoryManager::invalidateInstructionCache() {
-  for (auto &FM : FunctionMemory)
-    sys::Memory::InvalidateInstructionCache(FM.base(), FM.size());
-
-  for (auto &DM : DataMemory)
-    sys::Memory::InvalidateInstructionCache(DM.base(), DM.size());
-}
-
 static const char *ProgramName;
 
 static void Message(const char *Type, const Twine &Msg) {
   errs() << ProgramName << ": " << Type << ": " << Msg << "\n";
 }
 
 static int Error(const Twine &Msg) {
   Message("error", Msg);
for (auto &File : InputFileList) {
 
     // Load the object file
     Dyld.loadObject(Obj);
     if (Dyld.hasError()) {
       return Error(Dyld.getErrorString());
     }
   }
 
-  // Resolve all the relocations we can.
-  Dyld.resolveRelocations();
-  // Clear instruction cache before code will be executed.
-  MemMgr.invalidateInstructionCache();
-
+  // Resove all the relocations we can.
   // FIXME: Error out if there are unresolved relocations.
+  Dyld.resolveRelocations();
 
   // Get the address of the entry point (_main by default).
   void *MainAddress = Dyld.getSymbolLocalAddress(EntryPoint);
   if (!MainAddress)
     return Error("no definition for '" + EntryPoint + "'");
 
   // Invalidate the instruction cache for each loaded function.
   for (auto &FM : MemMgr.FunctionMemory) {
+
     // Make sure the memory is executable.
+    // setExecutable will call InvalidateInstructionCache.
     std::string ErrorStr;
-    sys::Memory::InvalidateInstructionCache(FM.base(), FM.size());
     if (!sys::Memory::setExecutable(FM, &ErrorStr))
       return Error("unable to mark function executable: '" + ErrorStr + "'");
   }
 
   // Dispatch to _main().
   errs() << "loaded '" << EntryPoint << "' at: " << (void*)MainAddress << "\n";
 
   int (*Main)(int, const char**) =