[PATCH] D159520: [BOLT][AArch64] Fix instrumentation deadloop

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

Thank you for this patch!

One very minor comment: in the commit message, you refer to section E2.10.5 of the ArmARM. That section covers Load-Exclusive and Store-Exclusive instructions in AArch32 mode.
This patch targets AArch64. The corresponding section (for AArch64) in the ArmARM seems to be "B2 <https://reviews.llvm.org/B2>.9.5 Load-Exclusive and Store-Exclusive instruction usage restrictions".



================
Comment at: bolt/lib/Passes/Instrumentation.cpp:295
+  // instruction. So for now skip instrumentation for functions that have
+  // these instructions, since it might lead to runtime deadlock.
+  if (BC.isAArch64()) {
----------------
I'm afraid I'm not familiar with the instrumentation feature of BOLT.

Given that typically there are only few instructions (and as you write above, no memory-accessing instructions) in between a load-exclusive and a store-exclusive, I would expect that it should be possible to make most instrumentation work well, even in functions with load-exclusives and store-exclusives?

Maybe only instrumentation that tracks direct conditional branches may be affected?
And even then, maybe it wouldn't be too hard to identify the conditional branches that are between a load-exclusive and a store-exclusive and only stop instrumenting those conditional branches, but still instrument the other conditional branches in the function?

Anyway, to me it feels like that's an improvement that could be done in a follow-on patch?

If all of the above text seems sensible, maybe it would be best to add a "FIXME" comment here to indicate that with a bit more work most instrumentations in functions with load-exclusives/store-exclusives could still be made to work?



================
Comment at: bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp:292
+            Inst.getOpcode() == AArch64::STLXRB ||
+            Inst.getOpcode() == AArch64::CLREX);
+  }
----------------
It seems the following opcodes are missing?

LDAXPW, LDAXPX, LDXPW, LDXPX,
STLXPW, STLXPX, STXPW, STXPX.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159520