On Wed, Jan 17, 2018 at 3:55 PM, Richard Earnshaw
<richard.earns...@arm.com> wrote:
>
> This patch adds generic support for the new builtin
> __builtin_speculation_safe_load. It provides the overloading of the
> different access sizes and a default fall-back expansion for targets
> that do not support a mechanism for inhibiting speculation.
+ if (ignore)
+ {
+ warning_at (input_location, 0,
+ "result of __builtin_speculation_safe_load must be used to "
+ "ensure correct operation");
+ target = NULL;
+ }
This warning cannot be disabled but these kind of cases could appear via
path isolation or missed optimizations since memory optimizations do not
recognize such speculative loads. So - should it not be emitted way
earlier instead, like from the FEs or during some early warning pass?
In which case it could be an error as well, no?
+ set_mem_align (mem, MAX (GET_MODE_ALIGNMENT (mode),
+ get_pointer_alignment (arg0)));
err... so there's no way to do an unaligned speculation safe load? Using
just get_pointer_alignment would be safe here. Other builtins/IFNs explicitely
pass down the alignment and FEs generate such alignment from their
language constraints.
+ set_mem_alias_set (mem, get_alias_set (TREE_TYPE (TREE_TYPE (arg0))));
sorry, but pointer types are arbitrary in GIMPLE so this is clearly
wrong. Without
more information you have to use zero. Thus,
__builtin_speculation_safe_load ((int *)p, ...);
will do the wrong thing if the type of p is not int *.
+ /* Mark the memory access as volatile. We don't want the optimizers to
+ move it or otherwise substitue an alternative value. */
+ MEM_VOLATILE_P (mem) = 1;
how is moving or substituting in any way related to inhibiting speculation?
Why's this done for all targets rather than only those that need any such
mitigation?
I btw miss implementation for x86_64 (at least) where the agreed upon mitigation
is to insert 'lfence' before the load.
Any reason the builtin expansion is not fully left to the targets?
+/* Suppressing speculation. Users are expected to use the first (N)
+ variant, which will be translated internally into one of the other
+ types. */
+DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_LOAD_N, "speculation_safe_load",
+ BT_FN_VOID_VAR, ATTR_NULL)
+
+DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_LOAD_1, "speculation_safe_load_1",
+ BT_FN_I1_CONST_VPTR_CONST_VPTR_CONST_VPTR_VAR, ATTR_NULL)
+DEF_GCC_BUILTIN (BUILT_IN_SPECULATION_SAFE_LOAD_2, "speculation_safe_load_2",
+ BT_FN_I2_CONST_VPTR_CONST_VPTR_CONST_VPTR_VAR, ATTR_NULL)
any reason you are not simply using an internal function for the
non-_N variants?
Looks like you're closely following the atomic/sync builtins but those
predate IFNs I think.
They also have more constraints on alignment and use a restrictive alias set.
+rtx
+default_speculation_safe_load (machine_mode mode ATTRIBUTE_UNUSED,
+ rtx result, rtx mem, rtx lower_bound,
+ rtx upper_bound, rtx cmpptr, bool warn)
+{
+ rtx_code_label *done_label = gen_label_rtx ();
+ rtx_code_label *inrange_label = gen_label_rtx ();
+
+ if (warn)
+ warning_at
+ (input_location, 0,
+ "this target does not support anti-speculation operations. "
+ "Your program will still execute correctly, but speculation "
+ "will not be inhibited");
so there's no way to inhibit this warning than changing all targets?
Iff it is correct for a target to expand this to an unconditional load with a
preceeding fence why not have the generic target hook implementation
do exactly that - emit an unconditional move? Maybe I misunderstand
the clear words in the hook docs, but the builtin docs also say
the behavior is undefined if the builtin is executed when the condition
is false - which means traps are allowed.
Richard.
> * builtin-types.def (BT_FN_I1_CONST_VPTR_CONST_VPTR_CONST_VPTR_VAR):
> New builtin type signature.
> (BT_FN_I2_CONST_VPTR_CONST_VPTR_CONST_VPTR_VAR): Likewise.
> (BT_FN_I4_CONST_VPTR_CONST_VPTR_CONST_VPTR_VAR): Likewise.
> (BT_FN_I8_CONST_VPTR_CONST_VPTR_CONST_VPTR_VAR): Likewise.
> (BT_FN_I16_CONST_VPTR_CONST_VPTR_CONST_VPTR_VAR): Likewise.
> * builtins.def (BUILT_IN_SPECULATION_SAFE_LOAD_N): New builtin.
> (BUILT_IN_SPECULATION_SAFE_LOAD_1): Likewise.
> (BUILT_IN_SPECULATION_SAFE_LOAD_2): Likewise.
> (BUILT_IN_SPECULATION_SAFE_LOAD_4): Likewise.
> (BUILT_IN_SPECULATION_SAFE_LOAD_8): Likewise.
> (BUILT_IN_SPECULATION_SAFE_LOAD_16): Likewise.
> * target.def (speculation_safe_load): New hook.
> * doc/tm.texi.in (TARGET_SPECULATION_SAFE_LOAD): Add to
> documentation.
> * doc/tm.texi: Regenerated.
> * doc/cpp.texi: Document __HAVE_SPECULATION_SAFE_LOAD.
> * doc/extend.texi: Document __builtin_speculation_safe_load.
> * c-family/c-common.c (speculation_safe_load_resolve_size): New
> function.
> (speculation_safe_load_resolve_params): New function.
> (speculation_safe_load_resolve_return): New function.
> (resolve_overloaded_builtin): Handle overloading
> __builtin_speculation_safe_load.
> * builtins.c (expand_speculation_safe_load): New function.
> (expand_builtin): Handle new speculation-safe builtins.
> * targhooks.h (default_speculation_safe_load): Declare.
> * targhooks.c (default_speculation_safe_load): New function.
> ---
> gcc/builtin-types.def | 16 +++++
> gcc/builtins.c | 81 +++++++++++++++++++++++
> gcc/builtins.def | 17 +++++
> gcc/c-family/c-common.c | 152
> ++++++++++++++++++++++++++++++++++++++++++++
> gcc/c-family/c-cppbuiltin.c | 5 +-
> gcc/doc/cpp.texi | 4 ++
> gcc/doc/extend.texi | 68 ++++++++++++++++++++
> gcc/doc/tm.texi | 9 +++
> gcc/doc/tm.texi.in | 2 +
> gcc/target.def | 34 ++++++++++
> gcc/targhooks.c | 59 +++++++++++++++++
> gcc/targhooks.h | 3 +
> 12 files changed, 449 insertions(+), 1 deletion(-)
>
