On Mon, Jul 9, 2018 at 6:40 PM Richard Earnshaw
<richard.earns...@arm.com> wrote:
>
>
> This patch defines a new intrinsic function
> __builtin_speculation_safe_value. A generic default implementation is
> defined which will attempt to use the backend pattern
> "speculation_safe_barrier". If this pattern is not defined, or if it
> is not available, then the compiler will emit a warning, but
> compilation will continue.
>
> Note that the test spec-barrier-1.c will currently fail on all
> targets. This is deliberate, the failure will go away when
> appropriate action is taken for each target backend.
So given this series is supposed to be backported I question
+rtx
+default_speculation_safe_value (machine_mode mode ATTRIBUTE_UNUSED,
+ rtx result, rtx val,
+ rtx failval ATTRIBUTE_UNUSED)
+{
+ emit_move_insn (result, val);
+#ifdef HAVE_speculation_barrier
+ /* Assume the target knows what it is doing: if it defines a
+ speculation barrier, but it is not enabled, then assume that one
+ isn't needed. */
+ if (HAVE_speculation_barrier)
+ emit_insn (gen_speculation_barrier ());
+
+#else
+ warning_at (input_location, 0,
+ "this target does not define a speculation barrier; "
+ "your program will still execute correctly, but speculation "
+ "will not be inhibited");
+#endif
+ return result;
which makes all but aarch64 archs warn on __bultin_speculation_safe_value
uses, even those that do not suffer from Spectre like all those embedded targets
where implementations usually do not speculate at all.
In fact for those targets the builtin stays in the way of optimization on GIMPLE
as well so we should fold it away early if neither the target hook is
implemented
nor there is a speculation_barrier insn.
So, please make resolve_overloaded_builtin return a no-op on such targets
which means you can remove the above warning. Maybe such targets
shouldn't advertise / initialize the builtins at all?
The builtins also have no attributes which mean they are assumed to be
1) calling back into the CU via exported functions, 2) possibly throwing
exceptions, 3) affecting memory state. I think you at least want
to use ATTR_NOTHROW_LEAF_LIST.
The builtins are not designed to be optimization or memory barriers as
far as I can see and should thus be CONST as well.
BUILT_IN_SPECULATION_SAFE_VALUE_PTR is declared but
nowhere generated? Maybe
+ case BUILT_IN_SPECULATION_SAFE_VALUE_N:
+ {
+ int n = speculation_safe_value_resolve_size (function, params);
+ tree new_function, first_param, result;
+ enum built_in_function fncode;
+
+ if (n == -1)
+ return error_mark_node;
+ else if (n == 0)
+ fncode = (enum built_in_function)((int)orig_code + 1);
+ else
+ fncode
+ = (enum built_in_function)((int)orig_code + exact_log2 (n) + 2);
resolve_size does that? Why can that not return the built_in_function
itself or BUILT_IN_NONE on error to make that clearer?
Otherwise it looks reasonable but C FE maintainers should comment.
I miss C++ testcases (or rather testcases should be in c-c++-common).
Richard.
> gcc:
> * builtin-types.def (BT_FN_PTR_PTR_VAR): New function type.
> (BT_FN_I1_I1_VAR, BT_FN_I2_I2_VAR, BT_FN_I4_I4_VAR): Likewise.
> (BT_FN_I8_I8_VAR, BT_FN_I16_I16_VAR): Likewise.
> * builtins.def (BUILT_IN_SPECULATION_SAFE_VALUE_N): New builtin.
> (BUILT_IN_SPECULATION_SAFE_VALUE_PTR): New internal builtin.
> (BUILT_IN_SPECULATION_SAFE_VALUE_1): Likewise.
> (BUILT_IN_SPECULATION_SAFE_VALUE_2): Likewise.
> (BUILT_IN_SPECULATION_SAFE_VALUE_4): Likewise.
> (BUILT_IN_SPECULATION_SAFE_VALUE_8): Likewise.
> (BUILT_IN_SPECULATION_SAFE_VALUE_16): Likewise.
> * builtins.c (expand_speculation_safe_value): New function.
> (expand_builtin): Call it.
> * doc/cpp.texi: Document predefine __HAVE_SPECULATION_SAFE_VALUE.
> * doc/extend.texi: Document __builtin_speculation_safe_value.
> * doc/md.texi: Document "speculation_barrier" pattern.
> * doc/tm.texi.in: Pull in TARGET_SPECULATION_SAFE_VALUE.
> * doc/tm.texi: Regenerated.
> * target.def (speculation_safe_value): New hook.
> * targhooks.c (default_speculation_safe_value): New function.
> * targhooks.h (default_speculation_safe_value): Add prototype.
>
> c-family:
> * c-common.c (speculation_safe_resolve_size): New function.
> (speculation_safe_resolve_params): New function.
> (speculation_safe_resolve_return): New function.
> (resolve_overloaded_builtin): Handle __builtin_speculation_safe_value.
> * c-cppbuiltin.c (c_cpp_builtins): Add pre-define for
> __HAVE_SPECULATION_SAFE_VALUE.
>
> testsuite:
> * gcc.dg/spec-barrier-1.c: New test.
> * gcc.dg/spec-barrier-2.c: New test.
> * gcc.dg/spec-barrier-3.c: New test.
> ---
> gcc/builtin-types.def | 6 ++
> gcc/builtins.c | 57 ++++++++++++++
> gcc/builtins.def | 20 +++++
> gcc/c-family/c-common.c | 143
> ++++++++++++++++++++++++++++++++++
> gcc/c-family/c-cppbuiltin.c | 5 +-
> gcc/doc/cpp.texi | 4 +
> gcc/doc/extend.texi | 29 +++++++
> gcc/doc/md.texi | 15 ++++
> gcc/doc/tm.texi | 20 +++++
> gcc/doc/tm.texi.in | 2 +
> gcc/target.def | 23 ++++++
> gcc/targhooks.c | 27 +++++++
> gcc/targhooks.h | 2 +
> gcc/testsuite/gcc.dg/spec-barrier-1.c | 40 ++++++++++
> gcc/testsuite/gcc.dg/spec-barrier-2.c | 19 +++++
> gcc/testsuite/gcc.dg/spec-barrier-3.c | 13 ++++
> 16 files changed, 424 insertions(+), 1 deletion(-)
> create mode 100644 gcc/testsuite/gcc.dg/spec-barrier-1.c
> create mode 100644 gcc/testsuite/gcc.dg/spec-barrier-2.c
> create mode 100644 gcc/testsuite/gcc.dg/spec-barrier-3.c
>
