On Thu, Jan 18, 2018 at 1:44 PM, Richard Biener
<richard.guent...@gmail.com> wrote:
> 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.
Oh, and I think I'd like to see the builtins folded to regular MEMs very early
if the target doesn't provide TARGET_SPECULATION_SAFE_LOAD. Ideally
directly from where _n is disambiguated in the FE.
Richard.
> 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(-)
>>
