This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |
Other format: | [Raw text] |
On 09/13/2018 03:33 AM, Richard Sandiford wrote:
Heh heh. Academical or not, it seems like finding these UINT256 bugs in the future will be harder than me passing the correct inner sign now.Aldy Hernandez <aldyh@redhat.com> writes:On 09/12/2018 12:57 PM, Richard Sandiford wrote:Aldy Hernandez <aldyh@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?The problem is with things like UINT256 << UINT8 vs. UINT256 << INT8. A range of [128, 131] on the UINT8 would be represented using the same wide_ints as a range of [-128, -125] on the INT8, but the former is well-defined while the latter isn't. Only the TYPE_SIGN tells you which applies. The original code got this right, but the new code effectively assumes all shift amounts are signed, and so would treat UINT8 like INT8. OK, so no current target actually supports UINT256 AFAIK, so it might be academic. But the original point of wide-int.h was to support such wide types, so they could become a thing in future.
My tree is a mess right now, but I'll submit a fix next week reverting the inner sign discrepancy, while keeping the bits that remove the vrp_shift_undefined_p wrapper.
Thank you for your explanation. Aldy
Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|
Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |