| Message ID | mptftip4fvc.fsf@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | [rs6000] Use VIEW_CONVERT_EXPR to reinterpret vectors (PR 92515) | expand |
Hi Richard, On Fri, Nov 15, 2019 at 10:02:15AM +0000, Richard Sandiford wrote: > The new tree-cfg.c checking in r278245 tripped on folds of > ALTIVEC_BUILTIN_VPERM_*, which were using gimple_convert > rather than VIEW_CONVERT_EXPR to reinterpret the contents > of a vector as a different type. > > Spot-tested on powerpc64-linux-gnu. OK to install? That looks fine, thanks! Yes please, okay for trunk, and for backports as needed or wanted. Segher > 2019-11-15 Richard Sandiford <richard.sandiford@arm.com> > > gcc/ > PR target/92515 > * config/rs6000/rs6000-call.c (rs6000_gimple_fold_builtin): Use > VIEW_CONVERT_EXPR to reinterpret vectors as different types.
On Fri, Nov 15, 2019 at 11:02 AM Richard Sandiford <richard.sandiford@arm.com> wrote: > > The new tree-cfg.c checking in r278245 tripped on folds of > ALTIVEC_BUILTIN_VPERM_*, which were using gimple_convert > rather than VIEW_CONVERT_EXPR to reinterpret the contents > of a vector as a different type. > > Spot-tested on powerpc64-linux-gnu. OK to install? Hmm, I think we should fix this up in gimple_convert instead. Richard. > Richard > > (There are actually two bugs in PR92515, this only fixes one of them.) > > > 2019-11-15 Richard Sandiford <richard.sandiford@arm.com> > > gcc/ > PR target/92515 > * config/rs6000/rs6000-call.c (rs6000_gimple_fold_builtin): Use > VIEW_CONVERT_EXPR to reinterpret vectors as different types. > > Index: gcc/config/rs6000/rs6000-call.c > =================================================================== > --- gcc/config/rs6000/rs6000-call.c 2019-11-14 14:49:32.629684201 +0000 > +++ gcc/config/rs6000/rs6000-call.c 2019-11-15 10:00:03.413590914 +0000 > @@ -6173,13 +6173,16 @@ rs6000_gimple_fold_builtin (gimple_stmt_ > // convert arg0 and arg1 to match the type of the permute > // for the VEC_PERM_EXPR operation. > tree permute_type = (TREE_TYPE (permute)); > - tree arg0_ptype = gimple_convert (&stmts, loc, permute_type, arg0); > - tree arg1_ptype = gimple_convert (&stmts, loc, permute_type, arg1); > + tree arg0_ptype = gimple_build (&stmts, loc, VIEW_CONVERT_EXPR, > + permute_type, arg0); > + tree arg1_ptype = gimple_build (&stmts, loc, VIEW_CONVERT_EXPR, > + permute_type, arg1); > tree lhs_ptype = gimple_build (&stmts, loc, VEC_PERM_EXPR, > permute_type, arg0_ptype, arg1_ptype, > permute); > // Convert the result back to the desired lhs type upon completion. > - tree temp = gimple_convert (&stmts, loc, TREE_TYPE (lhs), lhs_ptype); > + tree temp = gimple_build (&stmts, loc, VIEW_CONVERT_EXPR, > + TREE_TYPE (lhs), lhs_ptype); > gsi_insert_seq_before (gsi, stmts, GSI_SAME_STMT); > g = gimple_build_assign (lhs, temp); > gimple_set_location (g, loc);
Richard Biener <richard.guenther@gmail.com> writes: > On Fri, Nov 15, 2019 at 11:02 AM Richard Sandiford > <richard.sandiford@arm.com> wrote: >> >> The new tree-cfg.c checking in r278245 tripped on folds of >> ALTIVEC_BUILTIN_VPERM_*, which were using gimple_convert >> rather than VIEW_CONVERT_EXPR to reinterpret the contents >> of a vector as a different type. >> >> Spot-tested on powerpc64-linux-gnu. OK to install? > > Hmm, I think we should fix this up in gimple_convert instead. I think that's going to lead to too much overloading of what "convert" means though. E.g. for V2HI->V2SI it's now a sign or zero extension, just like for HI->SI. Whereas V4HI->V2SI would pack two HIs into a single SI. If gimple_convert is going to be "helpful", there's the question of what it should do for something like SI->SF. Should it be a FLOAT_EXPR or a VIEW_CONVERT_EXPR? If gimple_convert of V4SI->V4SF acts like a VIEW_CONVERT_EXPR then it seems more consistent for SI->SF to do the same, which IMO would be surprising for a function called gimple_convert. (I always forget that FLOAT_EXPR has its own code and expect "convert" to mean what it does in C.) Whereas if SI->SF is a FLOAT_EXPR than presumably V4SI->V4SF should be too. So IMO it's good that callers are explicit about what they actually want, with gimple_convert having the same semantics as CONVERT_EXPR/NOP_EXPR. Thanks, Richard
On Fri, Nov 15, 2019 at 11:59 AM Richard Sandiford <richard.sandiford@arm.com> wrote: > > Richard Biener <richard.guenther@gmail.com> writes: > > On Fri, Nov 15, 2019 at 11:02 AM Richard Sandiford > > <richard.sandiford@arm.com> wrote: > >> > >> The new tree-cfg.c checking in r278245 tripped on folds of > >> ALTIVEC_BUILTIN_VPERM_*, which were using gimple_convert > >> rather than VIEW_CONVERT_EXPR to reinterpret the contents > >> of a vector as a different type. > >> > >> Spot-tested on powerpc64-linux-gnu. OK to install? > > > > Hmm, I think we should fix this up in gimple_convert instead. > > I think that's going to lead to too much overloading of what "convert" > means though. E.g. for V2HI->V2SI it's now a sign or zero extension, > just like for HI->SI. Whereas V4HI->V2SI would pack two HIs into a > single SI. > > If gimple_convert is going to be "helpful", there's the question > of what it should do for something like SI->SF. Should it be a > FLOAT_EXPR or a VIEW_CONVERT_EXPR? If gimple_convert of V4SI->V4SF > acts like a VIEW_CONVERT_EXPR then it seems more consistent for SI->SF > to do the same, which IMO would be surprising for a function called > gimple_convert. (I always forget that FLOAT_EXPR has its own code > and expect "convert" to mean what it does in C.) Whereas if SI->SF > is a FLOAT_EXPR than presumably V4SI->V4SF should be too. Hmm, indeed. > So IMO it's good that callers are explicit about what they actually want, > with gimple_convert having the same semantics as CONVERT_EXPR/NOP_EXPR. OK, I wonder how I got away with gimple_convert in epilogue generation but I guess I'm only ever doing sign conversions there and we seem to accept both NOP and VCE for that. Richard. > Thanks, > Richard
Index: gcc/config/rs6000/rs6000-call.c =================================================================== --- gcc/config/rs6000/rs6000-call.c 2019-11-14 14:49:32.629684201 +0000 +++ gcc/config/rs6000/rs6000-call.c 2019-11-15 10:00:03.413590914 +0000 @@ -6173,13 +6173,16 @@ rs6000_gimple_fold_builtin (gimple_stmt_ // convert arg0 and arg1 to match the type of the permute // for the VEC_PERM_EXPR operation. tree permute_type = (TREE_TYPE (permute)); - tree arg0_ptype = gimple_convert (&stmts, loc, permute_type, arg0); - tree arg1_ptype = gimple_convert (&stmts, loc, permute_type, arg1); + tree arg0_ptype = gimple_build (&stmts, loc, VIEW_CONVERT_EXPR, + permute_type, arg0); + tree arg1_ptype = gimple_build (&stmts, loc, VIEW_CONVERT_EXPR, + permute_type, arg1); tree lhs_ptype = gimple_build (&stmts, loc, VEC_PERM_EXPR, permute_type, arg0_ptype, arg1_ptype, permute); // Convert the result back to the desired lhs type upon completion. - tree temp = gimple_convert (&stmts, loc, TREE_TYPE (lhs), lhs_ptype); + tree temp = gimple_build (&stmts, loc, VIEW_CONVERT_EXPR, + TREE_TYPE (lhs), lhs_ptype); gsi_insert_seq_before (gsi, stmts, GSI_SAME_STMT); g = gimple_build_assign (lhs, temp); gimple_set_location (g, loc);