[MIPS64] Make __clear_cache more optimal

[petarj]

Summary

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

[petarj]

Tested on MIPS64 Android port.

[joerg]

lib/builtins/clear_cache.c

27 ↗

(On Diff #17293)

Missing static to not pollute global namespace?

31 ↗

(On Diff #17293)

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

[petarj]

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

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

[petarj]

lib/builtins/clear_cache.c

27 ↗

(On Diff #17293)

Adding static will trigger a warning:

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

31 ↗

(On Diff #17293)

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"

[petarj]

Joerg? Dan?

[joerg]

lib/builtins/clear_cache.c

27 ↗

(On Diff #17293)

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

31 ↗

(On Diff #17293)

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]

lib/builtins/clear_cache.c

27 ↗

(On Diff #17293)

No, it will not.

31 ↗

(On Diff #17293)

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.

[petarj]

ping.

[petarj]

OK to commit?

[joerg]

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]

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]

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

[joerg]

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

lib/builtins/clear_cache.c

59 ↗

(On Diff #18169)

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

[petarj]

The comment has been added.

[petarj]

lib/builtins/clear_cache.c

59 ↗

(On Diff #18169)

Done.

[petarj]

OK to commit now?

[petarj]

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

[danalbert]

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?

[petarj]

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

compiler-rt/trunk/lib/builtins/clear_cache.c

 #endif
 
 #if defined(__NetBSD__) && defined(__arm__)
   #include <machine/sysarch.h>
 #endif
 
 #if defined(__ANDROID__) && defined(__mips__)
   #include <sys/cachectl.h>
+  #include <sys/syscall.h>
+  #ifdef __LP64__
+    /*
+     * clear_mips_cache - Invalidates instruction cache for Mips.
+     */
+    static void clear_mips_cache(const void* Addr, size_t Size) {
+      asm volatile (
+        ".set push\n"
+        ".set noreorder\n"
+        ".set noat\n"
+        "beq %[Size], $zero, 20f\n"          /* If size == 0, branch around. */
+        "nop\n"
+        "daddu %[Size], %[Addr], %[Size]\n"  /* Calculate end address + 1 */
+        "rdhwr $v0, $1\n"                    /* Get step size for SYNCI.
+                                                $1 is $HW_SYNCI_Step */
+        "beq $v0, $zero, 20f\n"              /* If no caches require
+                                                synchronization, branch
+                                                around. */
+        "nop\n"
+        "10:\n"
+        "synci 0(%[Addr])\n"                 /* Synchronize all caches around
+                                                address. */
+        "daddu %[Addr], %[Addr], $v0\n"      /* Add step size. */
+        "sltu $at, %[Addr], %[Size]\n"       /* Compare current with end
+                                                address. */
+        "bne $at, $zero, 10b\n"              /* Branch if more to do. */
+        "nop\n"
+        "sync\n"                             /* Clear memory hazards. */
+        "20:\n"
+        "bal 30f\n"
+        "nop\n"
+        "30:\n"
+        "daddiu $ra, $ra, 12\n"              /* $ra has a value of $pc here.
+                                                Add offset of 12 to point to the
+                                                instruction after the last nop.
+                                              */
+        "jr.hb $ra\n"                        /* Return, clearing instruction
+                                                hazards. */
+        "nop\n"
+        ".set pop\n"
+        : [Addr] "+r"(Addr), [Size] "+r"(Size)
+        :: "at", "ra", "v0", "memory"
+      );
+    }
+  #endif
 #endif
 
 #if defined(__ANDROID__) && defined(__arm__)
   #include <asm/unistd.h>
 #endif
 
 /*
  * The compiler generates calls to __clear_cache() when creating 
#elif defined(__ANDROID__)
              compilerrt_abort();
          }
     #else
         compilerrt_abort();
     #endif
 #elif defined(__ANDROID__) && defined(__mips__)
   const uintptr_t start_int = (uintptr_t) start;
   const uintptr_t end_int = (uintptr_t) end;
-  _flush_cache(start, (end_int - start_int), BCACHE);
+    #ifdef __LP64__
+        // Call synci implementation for short address range.
+        const uintptr_t address_range_limit = 256;
+        if ((end_int - start_int) <= address_range_limit) {
+            clear_mips_cache(start, (end_int - start_int));
+        } else {
+            syscall(__NR_cacheflush, start, (end_int - start_int), BCACHE);
+        }
+    #else
+        syscall(__NR_cacheflush, start, (end_int - start_int), BCACHE);
+    #endif
 #elif defined(__aarch64__) && !defined(__APPLE__)
   uint64_t xstart = (uint64_t)(uintptr_t) start;
   uint64_t xend = (uint64_t)(uintptr_t) end;
 
   // Get Cache Type Info
   uint64_t ctr_el0;
   __asm __volatile("mrs %0, ctr_el0" : "=r"(ctr_el0));