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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to