On Tue, 7 Jun 2022, Richard Sandiford wrote: > Joel Hutton <joel.hut...@arm.com> writes: > >> > Patches attached. They already incorporated the .cc rename, now > >> > rebased to be after the change to tree.h > >> > >> @@ -1412,8 +1412,7 @@ vect_recog_widen_op_pattern (vec_info *vinfo, > >> 2, oprnd, half_type, unprom, vectype); > >> > >> tree var = vect_recog_temp_ssa_var (itype, NULL); > >> - gimple *pattern_stmt = gimple_build_assign (var, wide_code, > >> - oprnd[0], oprnd[1]); > >> + gimple *pattern_stmt = gimple_build (var, wide_code, oprnd[0], > >> oprnd[1]); > >> > >> > >> you should be able to do without the new gimple_build overload > >> by using > >> > >> gimple_seq stmts = NULL; > >> gimple_build (&stmts, wide_code, itype, oprnd[0], oprnd[1]); > >> gimple *pattern_stmt = gimple_seq_last_stmt (stmts); > >> > >> because 'gimple_build' is an existing API. > > > > Done. > > > > The gimple_build overload was at the request of Richard Sandiford, I assume > > removing it is ok with you Richard S? > > From Richard Sandiford: > >> For example, I think we should hide this inside a new: > >> > >> gimple_build (var, wide_code, oprnd[0], oprnd[1]); > >> > >> that works directly on code_helper, similarly to the new code_helper > >> gimple_build interfaces. > > I thought the potential problem with the above is that gimple_build > is a folding interface, so in principle it's allowed to return an > existing SSA_NAME set by an existing statement (or even a constant). > I think in this context we do need to force a new statement to be > created.
Yes, that's due to how we use vect_finish_stmt_generation (only?). It might be useful to add an overload that takes a gimple_seq instead of a single gimple * for the vectorized stmt and leave all the magic to that. Now - we have the additional issue that we have STMT_VINFO_VEC_STMTS instead of STMT_VINFO_VEC_DEFS (in the end we'll only ever need the defs, never the stmts I think). I do think that we eventually want to 'enhance' the gimple.h non-folding stmt building API, unfortunately I took the 'gimple_build' name for the folding one, so alternatively we can unify assign/call with gimple_build_assign_or_call (...). I don't really like the idea of having folding and non-folding APIs being overloads :/ Maybe the non-folding API should be CTORs (guess GTY won't like that) or static member functions: gimple *gimple::build (tree, code_helper, tree, tree); and in the long run the gimple_build API should be (for some uses?) off a class as well, like instead of gimple_seq seq = NULL; op = gimple_build (&seq, ...); do gimple_builder b (location); // location defaulted to UNKNOWN op = b.build (...); So - writing the above I somewhat like the idea of static member functions in 'gimple' (yes, at the root of the class hierarchy, definitely not at gimple_statement_with_memory_ops_base, not sure if we want gassign::build for assigns and only the code_helper 'overloads' at the class root). Richard. -- Richard Biener <rguent...@suse.de> SUSE Software Solutions Germany GmbH, Frankenstraße 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)
