On 09/12/2018 12:57 PM, Richard Sandiford wrote:
Aldy Hernandez <al...@redhat.com> writes:
diff --git a/gcc/wide-int-range.h b/gcc/wide-int-range.h
index 589fdea4df6..e9ee418e5b2 100644
--- a/gcc/wide-int-range.h
+++ b/gcc/wide-int-range.h
@@ -131,7 +131,7 @@ extern bool wide_int_range_div (wide_int &wmin, wide_int
&wmax,
/* Return TRUE if shifting by range [MIN, MAX] is undefined behavior. */
inline bool
-wide_int_range_shift_undefined_p (signop sign, unsigned prec,
+wide_int_range_shift_undefined_p (unsigned prec,
const wide_int &min, const wide_int &max)
{
/* ?? Note: The original comment said this only applied to
@@ -142,7 +142,7 @@ wide_int_range_shift_undefined_p (signop sign, unsigned
prec,
behavior from the shift operation. We cannot even trust
SHIFT_COUNT_TRUNCATED at this stage, because that applies to rtl
shifts, and the operation at the tree level may be widened. */
- return wi::lt_p (min, 0, sign) || wi::ge_p (max, prec, sign);
+ return wi::sign_mask (min) || wi::ge_p (max, prec, UNSIGNED);
I don't think this is a good idea. Logically the comparison should
be done relative to the TYPE_SIGN of the type, so I think the original
code was correct.
The operation to calculate undefinedness must be done with the type of
the RHS, as opposed to the type of the entire operation. This can be
confusing, as most operations use the same type for all operands as well
as for the type of the entire operation. For example, AFAICT, the
following is valid gimple:
UINT64 = UINT64 << INT32
The original code was doing this (correctly), but since it was confusing
to remember which type to pass, I rewrote the above function to not need
the sign of the RHS. This came about because in my ranger work, I
passed the wrong type which took forever to find ;-). My patch avoids
further confusion.
Am I missing a subtle incorrectness in my approach?
Aldy
