[PATCH] D151942: [BOLT] Instrumentation: AArch64 instrumentation support in runtime

[Rafael Auler via Phabricator via llvm-commits]

rafauler added inline comments.


================
Comment at: bolt/CMakeLists.txt:34-35
 
-set(BOLT_ENABLE_RUNTIME_default OFF)
-if (CMAKE_SYSTEM_PROCESSOR STREQUAL "x86_64"
-    AND (CMAKE_SYSTEM_NAME STREQUAL "Linux"
-      OR CMAKE_SYSTEM_NAME STREQUAL "Darwin")
-    AND "X86" IN_LIST BOLT_TARGETS_TO_BUILD)
-  set(BOLT_ENABLE_RUNTIME_default ON)
-endif()
-option(BOLT_ENABLE_RUNTIME "Enable BOLT runtime" ${BOLT_ENABLE_RUNTIME_default})
+set(BOLT_ENABLE_RUNTIME ON)
+message(STATUS "Enable BOLT runtime by default")
 if (BOLT_ENABLE_RUNTIME)
----------------
We don't support runtime on windows, so we shouldn't delete these checks. Just change the x86 check to check for both x86 OR AArch64


================
Comment at: bolt/runtime/instr.cpp:1591
+#if defined(__aarch64__)
+  __asm__ __volatile__(SAVE_ALL "ldp x0, x1, [sp, #288]\n"
+                                "bl instrumentIndirectCall\n" RESTORE_ALL
----------------
I have the impression that the correct offset here is 256 instead of 288, but I can't test this. That's why I asked for a test for indirect calls in the other diff.


================
Comment at: bolt/runtime/instr.cpp:1609
+#if defined(__aarch64__)
+  __asm__ __volatile__(SAVE_ALL "ldp x0, x1, [sp, #288]\n"
+                                "bl instrumentIndirectCall\n" RESTORE_ALL
----------------
same


================
Comment at: bolt/runtime/instr.cpp:1614
+#else
+  __asm__ __volatile__(SAVE_ALL "mov 0x98(%%rsp), %%rdi\n"
+                                "mov 0x90(%%rsp), %%rsi\n"
----------------
clang format is doing the wrong thing here putting SAVE_ALL in the same line as other assembly instructions.

I think we should use: 
  // clang-format off

and
  // clang-format on

surrounding this code, and put SAVE_ALL in a separate line like before this diff.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151942/new/

https://reviews.llvm.org/D151942