[bolt][aarch64] Skip BB instrumentation with exclusive load/store instructions

[maksimra]

Basic blocks with exclusive load/store instructions are skipped from instrumentation for aarch64. In case of non-conservative mode the spanning tree leaves BB must be skipped either if contains exclusive instructions.

Related to #153492

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-bolt

Author: RachinskiyMaksim (maksimra)

Changes

Basic blocks with exclusive load/store instructions are skipped from instrumentation for aarch64. In case of non-conservative mode the spanning tree leaves BB must be skipped either if contains exclusive instructions.

Related to #153492

---

Full diff: https://github.com/llvm/llvm-project/pull/154734.diff

1 Files Affected:

• (modified) bolt/lib/Passes/Instrumentation.cpp (+2)

diff --git a/bolt/lib/Passes/Instrumentation.cpp b/bolt/lib/Passes/Instrumentation.cpp
index c2f876f0dff9e..ab54c2d68565f 100644
--- a/bolt/lib/Passes/Instrumentation.cpp
+++ b/bolt/lib/Passes/Instrumentation.cpp
@@ -579,6 +579,8 @@ void Instrumentation::instrumentFunction(BinaryFunction &Function,
   if (!opts::ConservativeInstrumentation) {
     for (auto BBI = Function.begin(), BBE = Function.end(); BBI != BBE; ++BBI) {
       BinaryBasicBlock &BB = *BBI;
+      if (BBToSkip.find(&BB) != BBToSkip.end())
+        continue;
       if (STOutSet[&BB].size() == 0)
         instrumentLeafNode(BB, BB.begin(), IsLeafFunction, *FuncDesc,
                            BBToID[&BB]);

[github-actions]

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Developer Policy and LLVM Discourse for more information.

[rafaelauler]

Hello @maksimra,

Thanks for looking into this matter and submitting a fix!

I'm concerned that a spanning tree-based reconstruction of edge counts depends on all leaf nodes being instrumented. If you omit one node, the reconstruction will corrupt random edge counts in a manner that is difficult to understand (as it depends on which nodes were skipped and how the spanning tree was constructed). This might create data that is not very useful for the purposes of basic block reordering, and it might be better to just rely on the original layout in these cases.

I have a few questions that would help me better understand this:

1. If using non-conservative mode, can't we skip the whole function? How prevalent are the exclusive load/store instructions? If they are rare, that would make the most sense for me.

2. If we can't skip the whole function, can we disable non-conservative mode entirely for Aarch64?

3. I assume this is hard or impossible, but for my education, is it possible to insert the instrumentation snippets even in basic blocks with exclusive load/store? What's stopping us from doing that?

Best,
Rafael

[yavtuk]

Hi @rafaelauler, great to hear you again, let me describe this because I asked Maxim to check this fix

1. If using non-conservative mode, can't we skip the whole function? How prevalent are the exclusive load/store instructions? If they are rare, that would make the most sense for me.

We skipped whole function before, but found that there are a lot of functions were skipped and our profile had the holes. We decided that it is possible to skip only the basic blocks between exclusive ldr and str/clear pair instructions.

2. If we can't skip the whole function, can we disable non-conservative mode entirely for Aarch64?

Will check this

3. I assume this is hard or impossible, but for my education, is it possible to insert the instrumentation snippets even in basic blocks with exclusive load/store? What's stopping us from doing that?

Based on available docs, the hardware block monitors the target load/store address and registers state are used this pair per core. I haven't found the info about it is possible or not to insert the load store instructions with another target address and registers but as we can see for the current example, the program is hanged, probably it is due to stadd instruction inside the snippet or monitor behaviour. The deep research related to exclusive load/store will be done near future. If you have any idea let's check this.

[maksimra]

Hi @rafaelauler. We have checked that disabling of conservative instrumentation solves this problem: instrumentation code snippet is no more inserted between exclusive load/store instructions.

[yavtuk]

related issue fixed by --conservative-instrumentation option usage