[BOLT] Enable hugify for AArch64 #117158
[BOLT] Enable hugify for AArch64 #117158
Conversation
alekuz01
requested review from
aaupov,
maksfb,
rafaelauler,
ayermolo and
dcci
as code owners
|
@llvm/pr-subscribers-bolt Author: alekuz01 (alekuz01) ChangesHi,
Enabling hugify aarch64. llvm-lit for runtime/AArch64 ninja check-bolt ` Total Discovered Tests: 472 Full diff: https://github.com/llvm/llvm-project/pull/117158.diff 7 Files Affected:
diff --git a/bolt/lib/Rewrite/RewriteInstance.cpp b/bolt/lib/Rewrite/RewriteInstance.cpp
index 7059a3dd231099..5345d90c4e4163 100644
--- a/bolt/lib/Rewrite/RewriteInstance.cpp
+++ b/bolt/lib/Rewrite/RewriteInstance.cpp
@@ -582,8 +582,12 @@ Error RewriteInstance::discoverStorage() {
// Hugify: Additional huge page from left side due to
// weird ASLR mapping addresses (4KB aligned)
- if (opts::Hugify && !BC->HasFixedLoadAddress)
+ if (opts::Hugify && !BC->HasFixedLoadAddress) {
NextAvailableAddress += BC->PageAlign;
+ BC->outs() << "BOLT-INFO: Hugify, Additional huge page from left side due to"
+ << "weird ASLR mapping addresses(4KB aligned): " << NextAvailableAddress
+ << '\n';
+ }
if (!opts::UseGnuStack && !BC->IsLinuxKernel) {
// This is where the black magic happens. Creating PHDR table in a segment
@@ -5722,17 +5726,22 @@ void RewriteInstance::rewriteFile() {
// Write all allocatable sections - reloc-mode text is written here as well
for (BinarySection &Section : BC->allocatableSections()) {
- if (!Section.isFinalized() || !Section.getOutputData())
+ if (!Section.isFinalized() || !Section.getOutputData()) {
+ BC->outs() << "BOLT: new section is finalized or !getOutputData, skip " << Section.getName() << '\n';
continue;
- if (Section.isLinkOnly())
+ }
+ if (Section.isLinkOnly()) {
+ BC->outs() << "BOLT: new section is link only, skip " << Section.getName() << '\n';
continue;
+ }
if (opts::Verbosity >= 1)
BC->outs() << "BOLT: writing new section " << Section.getName()
<< "\n data at 0x"
<< Twine::utohexstr(Section.getAllocAddress()) << "\n of size "
<< Section.getOutputSize() << "\n at offset "
- << Section.getOutputFileOffset() << '\n';
+ << Section.getOutputFileOffset()
+ << " with content size " << Section.getOutputContents().size() << '\n';
OS.seek(Section.getOutputFileOffset());
Section.write(OS);
}
diff --git a/bolt/runtime/common.h b/bolt/runtime/common.h
index 9b9965bae524eb..593b7b91d20042 100644
--- a/bolt/runtime/common.h
+++ b/bolt/runtime/common.h
@@ -151,10 +151,12 @@ struct timespec {
uint64_t tv_nsec; /* nanoseconds */
};
-#if defined(__aarch64__)
+#if defined(__aarch64__) || defined(__arm64__)
#include "sys_aarch64.h"
-#else
+#elif defined(__x86_64__)
#include "sys_x86_64.h"
+#else
+ exit(1);
#endif
constexpr uint32_t BufSize = 10240;
diff --git a/bolt/runtime/hugify.cpp b/bolt/runtime/hugify.cpp
index 05c1be4f2d70ca..901a2d61e987dd 100644
--- a/bolt/runtime/hugify.cpp
+++ b/bolt/runtime/hugify.cpp
@@ -6,7 +6,9 @@
//
//===---------------------------------------------------------------------===//
-#if defined (__x86_64__) && !defined(__APPLE__)
+#if defined(__x86_64__) \
+ || ( defined(__aarch64__) || defined(__arm64__) ) \
+ && !defined(__APPLE__)
#include "common.h"
@@ -73,8 +75,11 @@ static bool hasPagecacheTHPSupport() {
if (Res < 0)
return false;
- if (!strStr(Buf, "[always]") && !strStr(Buf, "[madvise]"))
+ if (!strStr(Buf, "[always]") && !strStr(Buf, "[madvise]")) {
+ DEBUG(report(
+ "[hugify] THP support is not enabled.\n");)
return false;
+ }
struct KernelVersionTy {
uint32_t major;
@@ -168,10 +173,16 @@ extern "C" void __bolt_hugify_self_impl() {
extern "C" __attribute((naked)) void __bolt_hugify_self() {
#if defined(__x86_64__)
__asm__ __volatile__(SAVE_ALL "call __bolt_hugify_self_impl\n" RESTORE_ALL
- "jmp __bolt_hugify_start_program\n" ::
- :);
+ "jmp __bolt_hugify_start_program\n"
+ :::);
+#elif defined(__aarch64__) || defined(__arm64__)
+ __asm__ __volatile__(SAVE_ALL "bl __bolt_hugify_self_impl\n" RESTORE_ALL
+ "adrp x16, __bolt_hugify_start_program\n"
+ "add x16, x16, #:lo12:__bolt_hugify_start_program\n"
+ "br x16\n"
+ :::);
#else
- exit(1);
+ __exit(1);
#endif
}
#endif
diff --git a/bolt/test/runtime/AArch64/Inputs/user_func_order.txt b/bolt/test/runtime/AArch64/Inputs/user_func_order.txt
new file mode 100644
index 00000000000000..48b76cd35f44d4
--- /dev/null
+++ b/bolt/test/runtime/AArch64/Inputs/user_func_order.txt
@@ -0,0 +1,2 @@
+main
+fib
diff --git a/bolt/test/runtime/AArch64/hugify.c b/bolt/test/runtime/AArch64/hugify.c
new file mode 100644
index 00000000000000..a54b8952c7007d
--- /dev/null
+++ b/bolt/test/runtime/AArch64/hugify.c
@@ -0,0 +1,34 @@
+// Make sure BOLT correctly processes --hugify option
+
+#include <stdio.h>
+
+int g1 = 1;
+int g2;
+static int sg1 = 1;
+static int sg2;
+
+
+int main(int argc, char **argv) {
+ printf("Hello world %p = %d , %p = %d\n", &g1, g1, &sg1, sg1);
+ printf("%p = %d , %p = %d\n", &g2, g2, &sg2, sg2);
+ return 0;
+}
+
+/*
+REQUIRES: system-linux,bolt-runtime
+
+RUN: %clang %cflags -no-pie %s -o %t.nopie.exe -Wl,-q
+RUN: %clang %cflags -fpic -pie %s -o %t.pie.exe -Wl,-q
+
+RUN: llvm-bolt %t.nopie.exe --lite=0 -o %t.nopie --hugify
+RUN: llvm-bolt %t.pie.exe --lite=0 -o %t.pie --hugify
+
+RUN: %t.nopie | FileCheck %s -check-prefix=CHECK-NOPIE
+
+CHECK-NOPIE: Hello world
+
+RUN: %t.pie | FileCheck %s -check-prefix=CHECK-PIE
+
+CHECK-PIE: Hello world
+
+*/
diff --git a/bolt/test/runtime/AArch64/section-order.test b/bolt/test/runtime/AArch64/section-order.test
new file mode 100644
index 00000000000000..f98af04422f243
--- /dev/null
+++ b/bolt/test/runtime/AArch64/section-order.test
@@ -0,0 +1,10 @@
+REQUIRES: system-linux,bolt-runtime
+
+RUN: %clang %p/Inputs/basic-instrumentation.s -Wl,-q -o %t.exe
+RUN: llvm-bolt %t.exe -o %t --instrument --hugify -v 10 -debug 2>&1 | tee section_order.log
+RUN: llvm-readelf --section-headers %t | FileCheck %s
+
+## Verify that llvm-bolt outputs new sections in expected order.
+CHECK: .text.bolt.extra.1
+CHECK: .rodata.bolt.extra.1
+CHECK: .data.bolt.extra.1
diff --git a/bolt/test/runtime/AArch64/user-func-reorder.c b/bolt/test/runtime/AArch64/user-func-reorder.c
new file mode 100644
index 00000000000000..fcb92bca16259b
--- /dev/null
+++ b/bolt/test/runtime/AArch64/user-func-reorder.c
@@ -0,0 +1,44 @@
+/* Checks that BOLT correctly processes a user-provided function list file,
+ * reorder functions according to this list, update hot_start and hot_end
+ * symbols and insert a function to perform hot text mapping during program
+ * startup.
+ */
+#include <stdio.h>
+
+int foo(int x) {
+ return x + 1;
+}
+
+int fib(int x) {
+ if (x < 2)
+ return x;
+ return fib(x - 1) + fib(x - 2);
+}
+
+int bar(int x) {
+ return x - 1;
+}
+
+int main(int argc, char **argv) {
+ printf("fib(%d) = %d\n", argc, fib(argc));
+ return 0;
+}
+
+/*
+REQUIRES: system-linux,bolt-runtime
+
+RUN: %clang %cflags -no-pie %s -o %t.exe -Wl,-q
+
+RUN: llvm-bolt %t.exe --relocs=1 --lite --reorder-functions=user \
+RUN: --hugify --function-order=%p/Inputs/user_func_order.txt -o %t
+RUN: llvm-nm --numeric-sort --print-armap %t | \
+RUN: FileCheck %s -check-prefix=CHECK-NM
+RUN: %t 1 2 3 | FileCheck %s -check-prefix=CHECK-OUTPUT
+
+CHECK-NM: W __hot_start
+CHECK-NM: T main
+CHECK-NM-NEXT: T fib
+CHECK-NM-NEXT: W __hot_end
+
+CHECK-OUTPUT: fib(4) = 3
+*/
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
we can move bolt/test/runtime/AArch64/hugify.c -> bolt/test/runtime/hugify.c and use the test for both platforms |
|
I would like to clarify a possibly more general question with community here, as I believe it might also come up with the AArch64 BOLT python tests for #120267 where we might need to test the same thing as existing X86 tests. What would you recommend, how would be better for us to organize such tests, what approach to follow? Things like when we need to duplicate them or keep common part target independent part and add differences for targets, make existing tests target independent to test all platforms, create unique AArch64 tests, etc? We would greatly appreciate any input. @yota9 @yavtuk |
|
From my point of view it depends on what functionality is implemented, bolt/test/runtime/hugify.c if the test will check a hardware specific implementation, bolt/test/runtime/aarch64/hugify.c but it must contain the checking for adrp + add instrs. Regarding the test #120267, it contains aarch64 specific checking for relocation
I think it should be under aarch64 directory. if this test platform independent than CHECK-RELOCS should be removed
|
|
@yavtuk really appreciate your comments. I have looked at your test as best practice right here https://github.com/llvm/llvm-project/blob/1fb186198af5f183dde053c1396f899567755d64/bolt/test/runtime/X86/hugify.c |
|
@alekuz01 Thanks for unification the tests in one place LGTM |
alekuz01
force-pushed
the
make-hugify-available-aarhc64
branch
from
f833601 to
e669603
Compare
bolt/lib/Rewrite/RewriteInstance.cpp
Outdated
| NextAvailableAddress += BC->PageAlign; | ||
| if (opts::Verbosity >= 1) { | ||
| BC->outs() |
There was a problem hiding this comment.
Not sure if we need this information. At least not under LLVM_DEBUG
There was a problem hiding this comment.
Sure! Interesting point you proposed. It is unclear how could it be more helpful, could you elaborate on this to get more context regarding general functionality. Hope the response to this comment will be prompt and helpful as usual.
There was a problem hiding this comment.
Sure, if you would first tell why we need this garbage information in the output :)
There was a problem hiding this comment.
Sure! Because it helps to inform about some issues that could have happened, and regarding garbage. Is it something you correlate with other Outputs under opts::Verbosity like this one for example:
So, it would be nice to discuss then what "other garbage" in your opinion are there?
| @@ -11,17 +11,28 @@ int main(int argc, char **argv) { | |||
| REQUIRES: system-linux,bolt-runtime | |||
|
|
|||
| RUN: %clang %cflags -no-pie %s -o %t.nopie.exe -Wl,-q | |||
| RUN: %clang %cflags -fpic -pie %s -o %t.pie.exe -Wl,-q | |||
There was a problem hiding this comment.
Sure! You may ask. I hope I could respond to this comment with the following explanation:
- llvm uses lit for testing
- lit has its own configuration files, for example lit.local.cfg (bolt/runtime, bolt/test)
- if you can check those files you can find there:
-pie -fPIEflags. Those flags are applied to the mentioned binary by default. Except-fpic.
+ bin/clang --target=aarch64-unknown-linux-gnu -fPIE -fuse-ld=lld -Wl,--unresolved-symbols=ignore-all -Wl,--build-id=none -pie llvm-project/bolt/test/runtime/hugify.c -o tools/bolt/test/runtime/Output/hugify.c.tmp.pie.exe -Wl,-q
I think your question is about whether to use -fpicor not in this case as -pie is used by default?
+ bin/clang --target=aarch64-unknown-linux-gnu -fPIE -fuse-ld=lld -Wl,--unresolved-symbols=ignore-all -Wl,--build-id=none -pie -fpic /llvm-project/bolt/test/runtime/hugify.c -o tools/bolt/test/runtime/Output/hugify.c.tmp.pie.exe -Wl,-q
Should we discuss what exactly -fpic would check here? Would it affect the output of nm?
| #include "sys_x86_64.h" | ||
| #else | ||
| #error "For AArch64/ARM64 and X86_64 only." |
There was a problem hiding this comment.
Nit: "Arch is not supported" probably better
There was a problem hiding this comment.
Thank you for your valuable comment. Let me disagree on this one with you.
There was a problem hiding this comment.
No problem, someone who would add support for RISC-V for example would absolutely change it "For AArch64/ARM64 and X86_64 and RISC-V only." as it is very convenient :)
There was a problem hiding this comment.
Sure! Why not? Or it just removes this line entirely because of this:
# Determine default set of targets to build -- the intersection of
# those BOLT supports and those LLVM is targeting.
set(BOLT_TARGETS_TO_BUILD_all "AArch64;X86;RISCV")
But if you worry about the cognitive effort that might be hard to understand as you demonstrated in your case:
"AArch64;X86;RISCV archs are supported!"
There was a problem hiding this comment.
Too pi ty that you don't understand that good code doesn't have to be changed each time. Not that I worried about my cognitive efforts, but your mental health. Sorry, not interested in further discussions with such an arrogant tool like you :)
There was a problem hiding this comment.
I do apologise for any inconvenience you have had because of this. However, Could you be more specific what exactly was so arrogant towards you based on fact that you have used words like "garbage" instead of more appropriate disagreement regarding appropriate style of error messages.
There was a problem hiding this comment.
Sure, I wrote exactly this word because of your previous comments about my "helpful and valuable comments" with barely hidden aggression. Please, don't play a victim now. Anyway it was addressed to output, not you or your work.
I'm not payed to work with you and just not interested in further discussions, maybe someone else dare to write any comments to your review if they're ready to have such a nice conversations.
Make debug message visible only when -debug used and verbosity > 1
Instead of fail exit we do nice macro message
Minor fix fto show a proper system arch for runtime libs
Fix clang-format.
Make hugify tests target independent
Add -fpic to the testin addition to -fPIE.
Remove bolt warning about additional huge page due to weird aslr mapping.
alekuz01
force-pushed
the
make-hugify-available-aarhc64
branch
from
025680f to
c308c33
Compare
|
@alekuz01 @yota9 thank you for the discussion and for sharing your perspectives. |
|
I think these tests are breaking on x64 Mac. Maybe there's some missing config/CMake plumbing? In any case, if the fix isn't simple, would you mind reverting until one is ready? |
Fix builds after llvm#117158: do not build hugify.cpp on Apple platforms.
Fix builds after #117158: do not build hugify.cpp on Apple platforms.
Add required hugify instrumentation and runtime libraries support for AArch64. Fixes llvm#58226 Unblocks llvm#62695
Fix builds after llvm#117158: do not build hugify.cpp on Apple platforms.
|
Hi @ilovepi , thanks for pointing out that x64 mac functionality is not working properly. Hope we could have mac testing on gh. I didn't have mac x64 to test it out properly. |
Hi,
To address following issues:
libbolt_rt_hugify.a&libbolt_rt_instr.aaren't generated on arm64 #58226Enabling hugify aarch64.
llvm-lit for runtime/AArch64
ninja check-bolt