On Thu, Apr 27, 2023 at 1:48 PM Alexandre Oliva via Gcc-patches
<[email protected]> wrote:
>
>
> The optimization barriers inserted after compares enable GCC to derive
> information about the values from e.g. the taken paths, or the absence
> of exceptions. Move them before the original compares, so that the
> reversed compares test copies of the original operands, without
> further optimizations.
>
> Regstrapped on x86_64-linux-gnu, and also bootstrapped with both passes
> enabled. Further tested on multiple other targets with gcc-12. Ok to
> install?
OK.
>
> for gcc/ChangeLog
>
> * gimple-harden-conditionals.cc (insert_edge_check_and_trap):
> Move detach value calls...
> (pass_harden_conditional_branches::execute): ... here.
> (pass_harden_compares::execute): Detach values before
> compares.
>
> for gcc/testsuite/ChangeLog
>
> * c-c++-common/torture/harden-cond-comp.c: New.
> ---
> gcc/gimple-harden-conditionals.cc | 25
> ++++++++++++--------
> .../c-c++-common/torture/harden-cond-comp.c | 24 +++++++++++++++++++
> 2 files changed, 39 insertions(+), 10 deletions(-)
> create mode 100644 gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
>
> diff --git a/gcc/gimple-harden-conditionals.cc
> b/gcc/gimple-harden-conditionals.cc
> index 78b8d5692d76f..2e5a42e9e71b1 100644
> --- a/gcc/gimple-harden-conditionals.cc
> +++ b/gcc/gimple-harden-conditionals.cc
> @@ -276,8 +276,8 @@ insert_check_and_trap (location_t loc,
> gimple_stmt_iterator *gsip,
> }
>
> /* Split edge E, and insert_check_and_trap (see above) in the
> - newly-created block, using detached copies of LHS's and RHS's
> - values (see detach_value above) for the COP compare. */
> + newly-created block, using already-detached copies of LHS's and
> + RHS's values (see detach_value above) for the COP compare. */
>
> static inline void
> insert_edge_check_and_trap (location_t loc, edge e,
> @@ -301,10 +301,6 @@ insert_edge_check_and_trap (location_t loc, edge e,
>
> gimple_stmt_iterator gsik = gsi_after_labels (chk);
>
> - bool same_p = (lhs == rhs);
> - lhs = detach_value (loc, &gsik, lhs);
> - rhs = same_p ? lhs : detach_value (loc, &gsik, rhs);
> -
> insert_check_and_trap (loc, &gsik, flags, cop, lhs, rhs);
> }
>
> @@ -366,6 +362,12 @@ pass_harden_conditional_branches::execute (function *fun)
> /* ??? Can we do better? */
> continue;
>
> + /* Detach the values before the compares. If we do so later,
> + the compiler may use values inferred from the compares. */
> + bool same_p = (lhs == rhs);
> + lhs = detach_value (loc, &gsi, lhs);
> + rhs = same_p ? lhs : detach_value (loc, &gsi, rhs);
> +
> insert_edge_check_and_trap (loc, EDGE_SUCC (bb, 0), cop, lhs, rhs);
> insert_edge_check_and_trap (loc, EDGE_SUCC (bb, 1), cop, lhs, rhs);
> }
> @@ -508,6 +510,13 @@ pass_harden_compares::execute (function *fun)
>
> tree rhs = copy_ssa_name (lhs);
>
> + /* Detach the values before the compares, so that the
> + compiler infers nothing from them, not even from a
> + throwing compare that didn't throw. */
> + bool same_p = (op1 == op2);
> + op1 = detach_value (loc, &gsi, op1);
> + op2 = same_p ? op1 : detach_value (loc, &gsi, op2);
> +
> gimple_stmt_iterator gsi_split = gsi;
> /* Don't separate the original assignment from debug stmts
> that might be associated with it, and arrange to split the
> @@ -529,10 +538,6 @@ pass_harden_compares::execute (function *fun)
> gimple_bb (asgn)->index, nbb->index);
> }
>
> - bool same_p = (op1 == op2);
> - op1 = detach_value (loc, &gsi_split, op1);
> - op2 = same_p ? op1 : detach_value (loc, &gsi_split, op2);
> -
> gassign *asgnck = gimple_build_assign (rhs, cop, op1, op2);
> gimple_set_location (asgnck, loc);
> gsi_insert_before (&gsi_split, asgnck, GSI_SAME_STMT);
> diff --git a/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> new file mode 100644
> index 0000000000000..5aad890a1d3b6
> --- /dev/null
> +++ b/gcc/testsuite/c-c++-common/torture/harden-cond-comp.c
> @@ -0,0 +1,24 @@
> +/* { dg-do compile } */
> +/* { dg-options "-fharden-conditional-branches -fharden-compares
> -fdump-tree-hardcbr -fdump-tree-hardcmp -ffat-lto-objects" } */
> +
> +int f(int i, int j) {
> + if (i == 0)
> + return j != 0;
> + else
> + return i * j != 0;
> +}
> +
> +/* { dg-final { scan-tree-dump-times "Splitting edge" 2 "hardcbr" } } */
> +/* { dg-final { scan-tree-dump-times "Adding reversed compare" 2 "hardcbr" }
> } */
> +/* { dg-final { scan-tree-dump-times "__builtin_trap" 2 "hardcbr" } } */
> +
> +/* { dg-final { scan-tree-dump-times "Splitting block" 2 "hardcmp" } } */
> +/* { dg-final { scan-tree-dump-times "Adding reversed compare" 2 "hardcmp" }
> } */
> +/* { dg-final { scan-tree-dump-times "__builtin_trap" 4 "hardcmp" } } */
> +
> +/* Check that the optimization barrier is placed before the original
> compare. */
> +/* { dg-final { scan-tree-dump-times {__asm__[(]"" : "=g" _[0-9]* : "0"
> i_[0-9]*[(]D[)][)][;][\n][ ]*if [(]i_[0-9]*[(]D[)] == 0[)]} 1 "hardcbr" } } */
> +/* { dg-final { scan-tree-dump-times {if [(]_[0-9]* != 0[)]} 2 "hardcbr" } }
> */
> +
> +/* { dg-final { scan-tree-dump-times {__asm__[(]"" : "=g" _[0-9]* : "0"
> j_[0-9]*[(]D[)][)][;][\n][ ]*_[0-9]* = j_[0-9]*[(]D[)] != 0;[\n] *_[0-9]* =
> _[0-9]* == 0} 1 "hardcmp" } } */
> +/* { dg-final { scan-tree-dump-times {__asm__[(]"" : "=g" _[0-9]* : "0"
> _[0-9]*[)][;][\n][ ]*_[0-9]* = _[0-9]* != 0;[\n] *_[0-9]* = _[0-9]* == 0} 1
> "hardcmp" } } */
>
>
> --
> Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/
> Free Software Activist GNU Toolchain Engineer
> Disinformation flourishes because many people care deeply about injustice
> but very few check the facts. Ask me about <https://stallmansupport.org>
