On Thu, Jan 27, 2022 at 8:14 PM Maciej W. Rozycki <ma...@embecosm.com> 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.
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
