Re: [PATCH v1 1/2] Simplify shifts wider than the bitwidth of types

[Philipp Tomsich <philipp.tomsich@vrull.eu>]

Jeff & Jakub,

I went back to reread the C language standard and it turns out that the
delineation between defined and undefined is not as simple as I thought
that I remembered (see below).

On Tue, 17 Nov 2020 at 17:54, Jeff Law <law@redhat.com> wrote:

>
>
> On 11/17/20 9:46 AM, Jakub Jelinek wrote:
> > On Tue, Nov 17, 2020 at 05:29:57PM +0100, Philipp Tomsich wrote:
> >>>> In other words, the change to VRP canonicalizes what a lshift_expr
> with an
> >>>> shift-amount outside of the type width means... it doesn't assume
> anything
> >>>> about the original language.
> >>>> Do we assume that a LSHIFT_EXPR has the same semantics as for a
> >>>> C-language shift-left? If so, then pre should not generate the
> LSHIFT_EXPR
> >>>> for _9... or we might even catch this later in path isolation (as
> >>>> undefined
> >>>> behavior, insert a __builtin_trap() and emit a warning)?
> >>>>
> >>>> Note that in his comment to patch 2/2, Jim has noted that user code
> for
> >>>> RISC-V may assume a truncation of the shift-operand...
> >>> What I'd suggest doing would be to leave the invalid shift count in the
> >>> IL in VRP, then extend the erroneous path isolation code to turn an
> >>> invalid shift into a trap (conditionally of course).
> >> You had originally suggested to add this to VRP ...
> >> Given the various comments to this patch, do you still want any of
> >> this in VRP or
> >> would you rather see this only in path isolation?
> > Well, I said if we want to do it at all, it should be done in VRP,
> because
> > there is not really a difference between ((int) x) << 32 and ((int) x)
> << y
> > for y in [32, 137] etc.
> Right.  VRP is the right place to discover, but I'm not sure it's the
> right place to clean up the mess though.
>

The rules for E1 << E2 are:
  - if E2 is negative => undefined
  - if E1 is unsigned => E1 x 2^E2, reduced module one more than the
maximum representable value
  - if E1 is signed and non-negative => E1 x 2^E2, if E1 x 2^E2 is
representable; otherwise, undefined

So the test case is 'undefined' due to E2 being negative.
However, if it was a large positive number, the transform would be valid if
E1 is unsigned.

I would propose the following revision to this patch:
 1. tighten the logic in VRP to handle the case of E1 being unsigned and E2
being positive...
 2. catch the undefined case of E2 being negative in path isolation

Agreed?
Philipp.