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

[Duncan P. N. Exon Smith via llvm-commits]

> On 2015-Nov-17, at 10:31, Davide Italiano <davide at freebsd.org> wrote:
> 
> 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.

Blank line at the beginning of a scope is strange to me since
the indentation already suggests "new paragraph".

Does clang-format remove the line?

>>> -  // 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.

This comment is not at the beginning of a scope, so I agree a
blank line is important.  clang-format would certainly leave
this one alone.

> What do you think?

I prefer no blank line at the beginning of a scope (and IIRC,
clang-format agrees with me), but if you prefer it this way and
clang-format doesn't care, then I don't care much either :).

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