On Fri, Jan 28, 2022 at 1:22 PM Richard Biener
<[email protected]> wrote:
>
> On Thu, Jan 27, 2022 at 8:14 PM Maciej W. Rozycki <[email protected]> wrote:
> >
> > On Thu, 27 Jan 2022, Richard Biener wrote:
> >
> > > > > > Index: gcc/gcc/c/c-typeck.cc
> > > > > > ===================================================================
> > > > > > --- gcc.orig/gcc/c/c-typeck.cc
> > > > > > +++ gcc/gcc/c/c-typeck.cc
> > > > > > @@ -49,6 +49,7 @@ along with GCC; see the file COPYING3.
> > > > > > #include "gomp-constants.h"
> > > > > > #include "spellcheck-tree.h"
> > > > > > #include "gcc-rich-location.h"
> > > > > > +#include "optabs-query.h"
> > > > > > #include "stringpool.h"
> > > > > > #include "attribs.h"
> > > > > > #include "asan.h"
> > > > > > @@ -11923,7 +11924,9 @@ build_binary_op (location_t location, en
> > > > > > bool maybe_const = true;
> > > > > > tree sc;
> > > > > > sc = c_fully_fold (op0, false, &maybe_const);
> > > > > > - sc = save_expr (sc);
> > > > > > + if (optab_handler (vec_duplicate_optab,
> > > > > > + TYPE_MODE (type1)) ==
> > > > > > CODE_FOR_nothing)
> > > > > > + sc = save_expr (sc);
> > > > >
> > > > > This doesn't make much sense - I suppose the CONSTRUCTOR retains
> > > > > TREE_SIDE_EFFECTS but such flag has no meaning on GIMPLE
> > > > > and thus should have been cleared during gimplification or in the end
> > > > > ignored by RTL expansion.
> > > >
> > > > This is how the expression built here eventually looks in
> > > > `store_constructor':
> > > >
> > > > (gdb) print exp
> > > > $41 = <constructor 0x7ffff5c06ba0>
> > > > (gdb) pt
> > > > <constructor 0x7ffff5c06ba0
> > > > type <vector_type 0x7ffff5e7ea48 v4sf
> > > > type <real_type 0x7ffff5cf1260 float sizes-gimplified SF
> > > > size <integer_cst 0x7ffff5c00f78 constant 32>
> > > > unit-size <integer_cst 0x7ffff5c00f90 constant 4>
> > > > align:32 warn_if_not_align:0 symtab:0 alias-set -1
> > > > canonical-type 0x7ffff5cf1260 precision:32
> > > > pointer_to_this <pointer_type 0x7ffff5cf1848>>
> > > > sizes-gimplified V4SF
> > > > size <integer_cst 0x7ffff5c00d80 constant 128>
> > > > unit-size <integer_cst 0x7ffff5c00d98 constant 16>
> > > > align:128 warn_if_not_align:0 symtab:0 alias-set -1
> > > > canonical-type 0x7ffff5d19648 nunits:4 context <translation_unit_decl
> > > > 0x7ffff5ec0bb8 v4sf-dup.c>>
> > > > side-effects length:4
> > > > val <ssa_name 0x7ffff5cb0dc8 type <real_type 0x7ffff5cf1260 float>
> > > > visited var <parm_decl 0x7ffff5f00080 y>
> > > > def_stmt GIMPLE_NOP
> > > > version:2>
> > > > val <ssa_name 0x7ffff5cb0dc8 type <real_type 0x7ffff5cf1260 float>
> > > > visited var <parm_decl 0x7ffff5f00080 y>
> > > > def_stmt GIMPLE_NOP
> > > > version:2>
> > > > val <ssa_name 0x7ffff5cb0dc8 type <real_type 0x7ffff5cf1260 float>
> > > > visited var <parm_decl 0x7ffff5f00080 y>
> > > > def_stmt GIMPLE_NOP
> > > > version:2>
> > > > val <ssa_name 0x7ffff5cb0dc8 type <real_type 0x7ffff5cf1260 float>
> > > > visited var <parm_decl 0x7ffff5f00080 y>
> > > > def_stmt GIMPLE_NOP
> > > > version:2>>
> > > > (gdb)
> > > >
> > > > The `side-effects' flag prevents this conditional from executing:
> > > >
> > > > /* Try using vec_duplicate_optab for uniform vectors. */
> > > > if (!TREE_SIDE_EFFECTS (exp)
> > > > && VECTOR_MODE_P (mode)
> > > > && eltmode == GET_MODE_INNER (mode)
> > > > && ((icode = optab_handler (vec_duplicate_optab, mode))
> > > > != CODE_FOR_nothing)
> > > > && (elt = uniform_vector_p (exp))
> > > > && !VECTOR_TYPE_P (TREE_TYPE (elt)))
> > > > {
> > > > class expand_operand ops[2];
> > > > create_output_operand (&ops[0], target, mode);
> > > > create_input_operand (&ops[1], expand_normal (elt),
> > > > eltmode);
> > > > expand_insn (icode, 2, ops);
> > > > if (!rtx_equal_p (target, ops[0].value))
> > > > emit_move_insn (target, ops[0].value);
> > > > break;
> > > > }
> > > >
> > > > I don't know what's supposed to clear the flag (and what the purpose of
> > > > setting it in the first place would be then).
> > >
> > > It's probably safe to remove the !TREE_SIDE_EFFECTS check above
> > > but already gimplification should have made sure all side-effects are
> > > pushed to separate stmts. gimplifiation usually calls
> > > recompute_side_effects
> > > but that doesn't seem to touch CONSTRUCTORs. But I do remember fixing
> > > some spurious TREE_SIDE_EFFECTS on CTORs before.
> > >
> > > Might be worth verifying in verify_gimple_assign_single that CTORs
> > > do not have TREE_SIDE_EFFECTS set (unless this is a clobber).
> >
> > OK, so maybe there's another bug somewhere that causes the side-effects
> > flag not to be cleared where expected, however I an inconvinced as to
> > withdrawing my original point. That is why treat code like:
> >
> > v4sf
> > odd_even (v4sf x, float y)
> > {
> > return x + f;
> > }
> >
> > effectively like:
> >
> > v4sf
> > odd_even (v4sf x, volatile float y)
> > {
> > return x + f;
> > }
>
> that's not what it does. It treats it like
>
> float tem = f;
> return x + { tem, tem, tem, tem };
>
> avoiding, like for x + (1.0f + f) creating
>
> return x + { 1.0+f, 1.0+f, 1.0+f ...}
>
> it's more CSE than volatile qualifying.
>
> > which I infer from the terse justification in the discussions referred is
> > the sole purpose of making use of `save_expr' here, also for targets that
> > have a cheap (or free if combined with another operation) `vec_duplicateM'
> > machine operation?
>
> Because the IL from the frontends should not depend on target capabilities
> and whether we have to preserve side-effects properly doesn't depend on
> the cheapness of the operation itself. Consider
>
> return x + bar (f);
>
> you definitely want bar(f) to be only evaluated once, even when the
> target can cheaply do the splat.
Btw,
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index efd10332c53..c0f7d98931d 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -4703,6 +4703,12 @@ verify_gimple_assign_single (gassign *stmt)
debug_generic_stmt (rhs1);
return true;
}
+ if (TREE_SIDE_EFFECTS (rhs1) && !gimple_clobber_p (stmt))
+ {
+ error ("%qs with side-effects", code_name);
+ debug_generic_stmt (rhs1);
+ return true;
+ }
return res;
case ASSERT_EXPR:
does not cause ICEs on the two testcases (on trunk).
Richard.
>
> Richard.
>
> > While removing the !TREE_SIDE_EFFECTS check may cause `vec_duplicateM' to
> > be used, the middle end will still ensure the element broadcast operation
> > won't be repeated, e.g. at the cost of consuming a temporary register to
> > carry a vector of identical elements, where it may not be the least costly
> > approach. Where we have an actual `vec_duplicateM' insn we can use its
> > cost to determine the best approach, can't we?
> >
> > Am I still missing something?
> >
> > Maciej
