From: Matthew Malcomson <mmalcom...@nvidia.com> Cc'ing in i386 maintainers.
-------------- >8 ------- 8< ----------- In ix86_atomic_assign_expand_fenv we generate some TREE to be expanded. Originally this code used `MODIFY_EXPR`, but that caused some bugs due to the use of `create_tmp_var_raw`. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94780 This was then fixed in revision 9b8e9006bb35 by using `TARGET_EXPR` instead. `MODIFY_EXPR` expressions are automatically marked with `TREE_SIDE_EFFECTS`, while `TARGET_EXPR` expressions are not. In most instances of this function this does not matter since the expression used to initialise variables is a function call and `build4` automatically sets the flag on the result anyway. For `hold_assign_orig` that is not the case. While it is initialised by a function call that function call is marked as PURE. Hence the initialisation is marked as not having side-effects. This variable eventually gets put into a `COMPOUND_EXPR` as the first argument with the second argument using the variable that has been added. This removal of the unused expression was not happening for the C frontend, but I newly use it in the C++ frontend where we call `cp_fold_r` and this exposes the latent bug. Here marking `hold_assign_orig` as `TREE_SIDE_EFFECTS` in order to avoid this problem. It will mean that the `COMPOUND_EXPR` it is used in is also marked as having side effects. While that means nothing gets optimised out in practice it still seems like good practice to mark `hold_assign_mod` as having side effects to. This assigns to `mxcsr_mod_var` which is used in later expressions. This commit fixes the below libstdc++ tests that fail with the previous commits in this patch series applied: - 29_atomics/atomic_float/1.cc - 29_atomics/atomic_float/compare_exchange_padding.cc - 29_atomics/atomic_ref/float.cc Questions for reviewers: - Is there any problem setting a `TARGET_EXPR` to have `TREE_SIDE_EFFECTS`? When trying to interpret why this expression doesn't automatically get set as having side effects I get the impression it's to do with the possible cacheing of the expression in argument 3 (honestly not 100% confident on my interpretation of what happens with this argument). - I would also appreciate a little expansion on why a `TARGET_EXPR` is not automatically marked as having side-effects in `make_node`, while `INIT_EXPR` is. I guess the operand 3 thing again? gcc/ChangeLog: * config/i386/i386.cc (ix86_atomic_assign_expand_fenv): Set side effects flag on `hold_assign_orig` and `hold_assign_mod`. Signed-off-by: Matthew Malcomson <mmalcom...@nvidia.com> --- gcc/config/i386/i386.cc | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc index 613f2b2139d..7ea56cd8d76 100644 --- a/gcc/config/i386/i386.cc +++ b/gcc/config/i386/i386.cc @@ -26737,6 +26737,14 @@ ix86_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update) tree hold_assign_orig = build4 (TARGET_EXPR, unsigned_type_node, mxcsr_orig_var, stmxcsr_hold_call, NULL_TREE, NULL_TREE); + /* Ensure that this doesn't get optimised out of the COMPOUND_EXPR we + * define below. It appears on first glance that the fact the + * initialisation argument is a function call would mean this + * automatically gets set, but that function call is marked as pure and + * so does not have the side-effects flag. + * This particular assignment needs to be marked as having side-effects + * because we use the value in the variable that gets assigned. */ + TREE_SIDE_EFFECTS (hold_assign_orig) = 1; tree hold_mod_val = build2 (BIT_IOR_EXPR, unsigned_type_node, mxcsr_orig_var, build_int_cst (unsigned_type_node, 0x1f80)); @@ -26745,6 +26753,15 @@ ix86_atomic_assign_expand_fenv (tree *hold, tree *clear, tree *update) tree hold_assign_mod = build4 (TARGET_EXPR, unsigned_type_node, mxcsr_mod_var, hold_mod_val, NULL_TREE, NULL_TREE); + /* Similar to above, ensure that this is marked as having side effects + * because we rely on the assignment to `mxcsr_mod_var` for later uses. + * While this expression is going in arg1 of a `COMPOUND_EXPR`, that + * `COMPOUND_EXPR` is going into arg0 of another one. Since we've marked + * the initialisation above as having side effects the `COMPOUND_EXPR` + * will not be optimised away and we don't technically need this second + * marking, however it still seems like good practice to mark everything + * that has side effects we care about as having those side effects. */ + TREE_SIDE_EFFECTS (hold_assign_mod) = 1; tree ldmxcsr_hold_call = build_call_expr (ldmxcsr, 1, mxcsr_mod_var); tree hold_all = build2 (COMPOUND_EXPR, unsigned_type_node, hold_assign_orig, hold_assign_mod); -- 2.43.0
