Jakub, On Tue, 17 Nov 2020 at 16:56, Jeff Law <l...@redhat.com> wrote: > > > > On 11/17/20 4:53 AM, Philipp Tomsich wrote: > > Jeff, > > > > On Tue, 17 Nov 2020 at 00:38, Jeff Law <l...@redhat.com > > <mailto:l...@redhat.com>> wrote: > > > > > > On 11/16/20 11:57 AM, Philipp Tomsich wrote: > > > From: Philipp Tomsich <p...@gnu.org <mailto:p...@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).
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? Philipp.
