[ Please Cc: me on combine patches. Thanks! ] On Thu, May 09, 2019 at 01:52:58PM +0200, Robin Dapp wrote: > I had already finished all the patterns when I realized that > SHIFT_COUNT_TRUNCATED and the target hook shift_truncation_mask already > exist and could do what is needed without extra patterns. Just defining > shift_truncation_mask was not enough though as most of the additional > insns get introduced by combine.
SHIFT_COUNT_TRUNCATED says that *all* of your (variable) shifts truncate the shift count in a particular way. targetm.shift_truncation_mask says that *all* of your variable shifts in a particular mode are done in a particular way. Neither is true, for some (many?) targets. For example, on Power, if done in the integer registers, simple shifts use one bit more than the datum rotated (this works for rotates as well of course). In the vector registers, you get a mask of just the size of the datum however. If it is not a simple shift, but a shift-and-mask, *nothing* works: for example, a DImode shift-and-mask with certain arguments is actually implemented with a 32-bit instruction (but many use a 64-bit instruction). It really depends on the machine instruction what the behaviour is, what the mask has to be; and for that at a minimum you need to know the icode, but many patterns have different machine insns in their alternatives, too. > While the attached patch might probably work for s390, it will probably > not for other targets. In addition to what my patch does, would it be > useful to unify both truncation methods in a target hook that takes the > operation (shift, rotate, zero_extract, ...) as well as the mode as > arguments? Thus, we would let the target decide what to do with the > specific combination of both. Maybe this would also allow to > distinguish bit test operations from the rest. Will this be useful for many targets? That's the question for all new hooks that aren't needed, that are just a nicety. It's a cost for everyone, not just your target, which is a bad tradeoff if it doesn't help enough targets, and helps enough. > @@ -12295,7 +12293,7 @@ simplify_comparison (enum rtx_code code, rtx *pop0, > rtx *pop1) > between the position and the location of the single bit. */ > /* Except we can't if SHIFT_COUNT_TRUNCATED is set, since we might > have already reduced the shift count modulo the word size. */ > - if (!SHIFT_COUNT_TRUNCATED > + if ((!SHIFT_COUNT_TRUNCATED || !targetm.shift_truncation_mask (mode)) Please make the (default) hook implementation use the macro, instead? > diff --git a/gcc/config/s390/s390.c b/gcc/config/s390/s390.c > index ad8eacdf4dc..1d723f29e1e 100644 > --- a/gcc/config/s390/s390.c > +++ b/gcc/config/s390/s390.c > @@ -2320,6 +2320,7 @@ s390_single_part (rtx op, > part = i; > } > } > + No spurious whitespace changes please. There is a lot that can be improved or should be fixed, but let's not do that one by one, and not in random patches. Segher
