Re: [PATCH] optabs: Fix up expand_doubleword_shift_condmove for shift_mask == 0 [PR108803]

[Jakub Jelinek <jakub@redhat.com>]

On Mon, Feb 27, 2023 at 09:01:15PM +0000, Richard Sandiford via Gcc-patches wrote:
> Jakub Jelinek <jakub@redhat.com> writes:
> > On Mon, Feb 27, 2023 at 08:43:27PM +0000, Richard Sandiford wrote:
> >> My argument was that !SHIFT_COUNT_TRUNCATED and
> >> C?Z_DEFINED_VALUE_AT_ZERO==0 mean that the behaviour is undefined
> >> only in the sense that target-independent code doesn't know what
> >> the behaviour is.  !SHIFT_COUNT_TRUNCATED doesn't mean that
> >> target-independent code can assume that out-of-range shift values
> >> invoke program UB (and therefore target-independent code can optimise
> >> shifts on the principle that all shifts are in-range).  Similarly
> >> CTZ_DEFINED_VALUE_AT_ZERO==0 doesn't mean the corresponding thing for CTZ.
> >
> > Even if the target-independent code doesn't know what the target dependent
> > code will do, I don't see how it could emit it safely.
> > In that case, I could understand some particular backend emitting such code
> > because it knows what to assume, and then generic code like simplify-rtx
> > simply trying to punt and not mess up with things which it doesn't know how
> > they'll behave.
> > But in this case it isn't aarch64 backend that emits this, but it is
> > the generic expansion code.  It even guarantees one of the two shifts
> > will be out of bounds...  And target didn't tell it that out of bound
> > shifts say won't trap or do something similar.
> 
> But we haven't had to handle trapping shifts because no supported target
> behaves like that.  And yeah, even if you disagree with that being a
> reasonable assumption...
> 
> > Sure, there could be a target hook or macro that would tell the expander
> > that the backend out of bound shift will just produce unspecified result
> > but not other effects.
> 
> ...adding an explicit control, or putting the expansion in aarch64 code,
> would allow us to generate the same RTL as we do now, and so hit the
> same bug.

Ok, but then we should document it (that at RTL an out of bounds shift
can only result in unspecified result value but not have other
side-effects).

Which would turn my patch from a correctness thing into an optimization
territory (especially when PR108840 will be fixed), because for
architectures which have these shifts with masking patterns it emits fewer
instructions.  Though, because for targets which don't do that it emits
more code, we'd need some target hook that could tell the expander
that is the case.  And of course make it a GCC14 material rather than GCC13.

	Jakub