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

[Jeff Law <law@redhat.com>]

On 11/17/20 4:53 AM, Philipp Tomsich wrote:
> Jeff,
>
> On Tue, 17 Nov 2020 at 00:38, Jeff Law <law@redhat.com
> <mailto:law@redhat.com>> wrote:
>
>
>     On 11/16/20 11:57 AM, Philipp Tomsich wrote:
>     > From: Philipp Tomsich <prt@gnu.org <mailto:prt@gnu.org>>
>     >
>     > While most shifts wider than the bitwidth of a type will be
>     caught by
>     > other passes, it is possible that these show up for VRP.
>     > Consider the following example:
>     >   int func (int a, int b, int c)
>     >   {
>     >     return (a << ((b && c) - 1));
>     >   }
>     >
>     > This adds simplify_using_ranges::simplify_lshift_using_ranges to
>     > detect and rewrite such cases.  If the intersection of meaningful
>     > shift amounts for the underlying type and the value-range computed
>     > for the shift-amount (whether an integer constant or a variable) is
>     > empty, the statement is replaced with the zero-constant of the same
>     > precision as the result.
>     >
>     > gcc/ChangeLog:
>     >
>     >        * vr-values.h (simplify_using_ranges): Declare.
>     >        * vr-values.c (simplify_lshift_using_ranges): New function.
>     >        (simplify): Use simplify_lshift_using_ranges for LSHIFT_EXPR.
>
>     Umm, isn't this a shift wider than the bitwidth undefined
>     behavior?  We
>     should be generating warnings for that, not trying to further optimize
>     it :-)
>
>
> The shift is undefined behavior on the language level (for C) and a
> warning
> will be generated, if such a shift is encountered; additionally, the
> shift will be
> replaced with the value 0.
>
> However, in the above case, the shift is generated only in the middle end:
> At 136t.walloca, I still have:
>
>       # RANGE [-1, 0]
>       _1 = iftmp.1_2 + -1;
>       _6 = a_5(D) << _1;
>
> Whereas at 137t.pre, this is changed into:
>
>     Found partial redundancy for expression {lshift_expr,a_5(D),_1} (0006)
>     Inserted _9 = a_5(D) << -1;
>
>
> 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).

jeff