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

[Vladislav Khmelevsky via Phabricator via llvm-commits]

yota9 marked 2 inline comments as done.
yota9 added a comment.

Thank you for your review, Kristof! My bad, you're right, at my document it is B2 <https://reviews.llvm.org/B2>.10.5 (Load-Exclusive and Store-Exclusive instruction usage restrictions), I would fix it in commit message.



================
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()) {
----------------
kristof.beyls wrote:
> 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?
> 
I would add FIXME as you suggested. 
As for the BOLT we're using multiple load/store instruction, e.g. to preserve the register values that we're using in snippets and for the atomic add for the execution counter. So every snippet would have load or store anyway


================
Comment at: bolt/lib/Target/AArch64/AArch64MCPlusBuilder.cpp:292
+            Inst.getOpcode() == AArch64::STLXRB ||
+            Inst.getOpcode() == AArch64::CLREX);
+  }
----------------
kristof.beyls wrote:
> It seems the following opcodes are missing?
> 
> LDAXPW, LDAXPX, LDXPW, LDXPX,
> STLXPW, STLXPX, STXPW, STXPX.
Forgot about pair ones, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159520