[PATCH] D55929: Initial AArch64 SLH implementation.

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls marked 3 inline comments as done.
kristof.beyls added a comment.

In D55929#1358920 <https://reviews.llvm.org/D55929#1358920>, @zbrid wrote:

> Hi Kristof,
>
> Thanks for writing this code! I had an easy time understanding what you were doing.
>
> I was discussing this SLH implementation with Chandler and he suggested that it may be useful to write a design doc similar to the one for the x86 implementation, so other people in the community can understand and discuss the current/future design with the ARM specific details laid out. What do you think?


I agree it would be good to have such a design doc. Actually I think it would be best to adapt https://llvm.org/docs/SpeculativeLoadHardening.html and separate out x86 and aarch64-specific implementation aspects from the target independent concepts there.
I am happy to try and do that, but will definitely not have time to do so this week. Maybe later this month.
I'd also be happy if e.g. you yourself would like to work on that.



================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:411
+    // Only harden loaded values or addresses used in loads.
+    if (!MI.mayLoad())
+      continue;
----------------
zbrid wrote:
> Right now, this masks after every load in a program. Is one of the future optimizations you mention in the comment in this file to mask only after loads that are depended upon by later non-data invariant operations?
Rereading the FIXMEs I've written in this function: no, that is not recorded as a potential future optimization.
Out of interest, is this optimization described in the design document at https://llvm.org/docs/SpeculativeLoadHardening.html somewhere?
I haven't thought about that optimization in detail yet and it seems a bit unclear to me why it is always safe to perform such an optimization.


================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64SpeculationHardening.cpp:447
+
+    if (HardenLoadedData)
+      for (auto Def : MI.defs()) {
----------------
zbrid wrote:
> I'm not sure if this case is possible, but is it possible that some defs are GPR and some aren't? If that's possible would it be worthwhile to harden the GPRs and non-GPRs in different ways? It appears to me that currently if any are non-GPR, then all of them are treated as non-GPRs.
Yeah, something like 
LDR D0, [X0], #8
would load a value into D0 and update the address in X0, so def-ing both D0 and X0.
However, with current code, X0 would be masked ("the address loaded from"/HardenAddressLoadedFrom).
If you already harden the address loaded from, there is no need to also harden the loaded data.
If you already have to harden the address loaded from, my guess is that there is not much opportunity for further optimizing/reducing the overhead of that masking.
But I am happy to be proven wrong - I haven't thought this through in detail.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55929