This is an archive of the discontinued LLVM Phabricator instance.

[MIPS64] Make __clear_cache more optimal
ClosedPublic

Authored by petarj on Dec 15 2014, 9:28 AM.

Details

Summary

Use synci implementation of clear_cache for short address ranges.
For long address ranges, make a kernel call.

Diff Detail

Event Timeline

petarj updated this revision to Diff 17293.Dec 15 2014, 9:28 AM
petarj retitled this revision from to [MIPS64] Make __clear_cache more optimal.
petarj updated this object.
petarj edited the test plan for this revision. (Show Details)
petarj added a reviewer: danalbert.
petarj added a subscriber: Unknown Object (MLST).

Tested on MIPS64 Android port.

joerg added a subscriber: joerg.Dec 15 2014, 9:45 AM
joerg added inline comments.
lib/builtins/clear_cache.c
32

Missing static to not pollute global namespace?

36

This should go away too. Why do you not use inline asm and let the compiler create the call and/or inline it? I'd prefer that...

Dan, this is a follow-up to your change [1] at AOSP.

[1] https://android-review.googlesource.com/#/c/105005/

petarj added inline comments.Dec 16 2014, 10:28 AM
lib/builtins/clear_cache.c
32

Adding static will trigger a warning:

warning: function 'clear_mips_cache' has internal linkage but is not defined [-Wundefined-internal]

36

This code needs to clear instruction hazards at its end, and using "jr.hb" is the most appropriate way to do so.
Making a version that could be inlined would, I believe, require a non-optimal trick to get pc value.
So, having this in a separate function is more optimal.
Last, the call to clear_mips_cache() will be accompanied with CALL16 relocations, and removing a .globl directive will trigger an error:

"CALL16 reloc at 0xAB not against global symbol"

Joerg? Dan?

joerg added inline comments.Dec 17 2014, 9:29 AM
lib/builtins/clear_cache.c
32

attribute((used)) should disable the warning again?

36

OK, so inline is not an option. That doesn't explain why it has to be global -- the warning above can be silenced, so what is the problem with declaring a file-local version?

petarj added inline comments.Dec 18 2014, 10:39 AM
lib/builtins/clear_cache.c
32

No, it will not.

36

OK, so inline is not an option.

Inline is an option, but this seems to be more clear solution. I can create an inlineable version if necessary.

That doesn't explain why it has to be global -- the warning above can be silenced,

Any other idea how?

so what is the problem with declaring a file-local version?

If we are going deeper in this discussion, the main issue will be that the bitcode does not hold the information that the function linkage type is internal, it leaves default linkage type and that is global. MIPS relocations are different for local and global symbols, so when the backend translates 'call clear_cache()', it checks its linkage type and emits a relocation for the global symbol (R_MIPS_CALL16) instead of relocations for local symbols (R_MIPS_GOT16 + R_MIPS_LO16). This error is later reported by the linker.

OK to commit?

Leaking clear_mips_cache is still absolutely not appropriate. I have been busy before Christmas. I'll try to come up with a better solution over the next week.

joerg added a comment.Jan 3 2015, 1:54 PM

What about using something like the following:

static void clear_mips_cache(const void* Addr, size_t Size) {
  __asm__("...\n"
                 "jr.hb %0" :: r(&&after);
after:
  ;
}

and not creating a separate function in asm at all?

petarj updated this revision to Diff 18169.Jan 14 2015, 11:31 AM

Here is a version that can be inlined. What do you think?

Can't comment on the correctness of the MIPS assembly, but this looks much better.

lib/builtins/clear_cache.c
59

I take it the 12 is the offset of the instruction just after the nop? If yes, please comment.

petarj updated this revision to Diff 18178.Jan 14 2015, 1:08 PM

The comment has been added.

petarj added inline comments.Jan 14 2015, 1:09 PM
lib/builtins/clear_cache.c
59

Done.

OK to commit now?

Joerg? Dan? If there are no further objections, I would commit this today.

danalbert edited edge metadata.Jan 16 2015, 11:40 AM

I can't speak to the MIPS asm either, but I do have one question: isn't this code useful for non-Android MIPS as well?

@danalbert, it is useful, and it will be enabled in later changes.
It shall also be followed with additional conditionals, like - check whether '-msynci' is set before we use it, etc.
The need for this change comes from Android build, thus we are making sure we fix Android build here first.

This revision was automatically updated to reflect the committed changes.