[llvm] r253343 - [llvm-rtdyld] Don't waste cycles invalidating instruction cache.

[Davide Italiano via llvm-commits]

On Tue, Nov 17, 2015 at 10:09 AM, Duncan P. N. Exon Smith
<dexonsmith at apple.com> wrote:
>
>> On 2015-Nov-17, at 08:37, Davide Italiano via llvm-commits <llvm-commits at lists.llvm.org> wrote:
>>
>> Author: davide
>> Date: Tue Nov 17 10:37:52 2015
>> New Revision: 253343
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=253343&view=rev
>> Log:
>> [llvm-rtdyld] Don't waste cycles invalidating instruction cache.
>>
>> Now that setExecutable() changed to do all the ground work to make
>> memory executable on the host, we can remove all (redundant) calls
>> to invalidate instruction cache here.
>>
>> As an added bonus, this makes invalidateInstructionCache() dead
>> code, so it can be removed.
>>
>> Differential Revision:        http://reviews.llvm.org/D13631
>>
>> Modified:
>>    llvm/trunk/tools/llvm-rtdyld/llvm-rtdyld.cpp
>>
>> Modified: llvm/trunk/tools/llvm-rtdyld/llvm-rtdyld.cpp
>> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/tools/llvm-rtdyld/llvm-rtdyld.cpp?rev=253343&r1=253342&r2=253343&view=diff
>> ==============================================================================
>> --- llvm/trunk/tools/llvm-rtdyld/llvm-rtdyld.cpp (original)
>> +++ llvm/trunk/tools/llvm-rtdyld/llvm-rtdyld.cpp Tue Nov 17 10:37:52 2015
>> @@ -155,12 +155,6 @@ public:
>>
>>   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;
>>   }
>> @@ -244,14 +238,6 @@ uint8_t *TrivialMemoryManager::allocateD
>>   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) {
>> @@ -424,12 +410,9 @@ static int executeInput() {
>>     }
>>   }
>>
>> -  // 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);
>> @@ -438,9 +421,10 @@ static int executeInput() {
>>
>>   // Invalidate the instruction cache for each loaded function.
>>   for (auto &FM : MemMgr.FunctionMemory) {
>> +
>
> Spurious blank line?
>

That was actually voluntary, but I'm not sure it's correct. I always
leave one blank line before any comment -- it's an habit I have from
working on other projects where it's an hard requirement. To be
honest, I like this better, and this is the first time somebody
noticed -- and I see there's inconsistency in the tree about this. If
that bothers you, I'll change it, but I'd rather do a sweep all over
this file to make it consistent, see e.g.

>>
>> -  // 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();
>>

a couple of lines above.
What do you think?

-- 
Davide

"There are no solved problems; there are only problems that are more
or less solved" -- Henri Poincare