[PATCH] D59827: [slh] x86 impl of ARM instrinsic + builtin for SLH

[Kristof Beyls via Phabricator via cfe-commits]

kristof.beyls added a comment.

This intrinsic got added to gcc a while ago and should become available in the upcoming gcc 9 release.
In gcc however, the prototype of the intrinsic is slightly different (see https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html):
type __builtin_speculation_safe_value (type val, type failval)
It provides a second optional argument "failval". From the gcc documentation: "The function may use target-dependent speculation tracking state to cause failval to be returned when it is known that speculative execution has incorrectly predicted a conditional branch operation."
So, when implementing the intrinsic using a speculation barrier such as lfence, that failval argument doesn't have any effect. However, when lowering the intrinsic using speculation tracking similar to how that's used in SLH, this failval parameter is used to return a non-zero value on miss-speculation, in case the developer prefers that over the default zero value.

I think we should make the intrinsic compatible with the one introduced in gcc.



================
Comment at: llvm/test/CodeGen/X86/speculative-load-hardening-intrinsic.ll:1
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu | FileCheck %s --check-prefix=X64
+
----------------
I guess the -mtriple command line option may not be needed since the IR file contain "target triple" and "target datalayout" information?


================
Comment at: llvm/test/CodeGen/X86/speculative-load-hardening-intrinsic.ll:3-4
+
+; ModuleID = 'hello.cpp'
+source_filename = "hello.cpp"
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
----------------
I guess this is not strictly necessary for this test, so should be removed?


================
Comment at: llvm/test/CodeGen/X86/speculative-load-hardening-intrinsic.ll:8-62
+; Function Attrs: noinline nounwind optnone uwtable
+define dso_local i32 @_Z5foo32i(i32 %a) #0 {
+entry:
+  %a.addr = alloca i32, align 4
+  %b = alloca i32, align 4
+  %b_safe = alloca i32, align 4
+  %c = alloca i32, align 4
----------------
Thanks for those updates, Zola. It makes it easier to compare this patch with the code I wrote earlier.
Doing that comparison, I see that I had a few changes too in target-independent SelectionDAG under lib/Codegen/SelectionDAG.
IIRC, you might find that you'll need that code if you also add tests here to test the correct thing happens when applying the intrinsic on other types than i32 or i64.
You probably also would want a test on a pointer data type here, I guess.


================
Comment at: llvm/test/CodeGen/X86/speculative-load-hardening-intrinsic.ll:64-71
+attributes #0 = { noinline nounwind optnone uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { nounwind }
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"wchar_size", i32 4}
----------------
I guess this is not strictly necessary for this test, so should be removed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59827