On 26/06/25 9:20 pm, Segher Boessenkool wrote:
> Hi!
>
> [ Please don't say "patch v1", it's just clutter. ]
>
> The point of the patch is to improve some code that evokes undefined
> behaviour. The sanitizer for that is how these problems were found,
> you can remark that somewhere later in the message, but ubsan is not
> what this patch is about, it does not belong in the title.
>
> On Thu, Jun 26, 2025 at 01:26:00PM +0530, Kishan Parmar wrote:
>> The following patch has been bootstrapped and regtested on powerpc64le-linux.
> Details like that belong at the *end* of a patch message. They do not
> usually belong in a commit message at all.
Got it. will take care going forward.
>> --- a/gcc/config/rs6000/rs6000.cc
>> +++ b/gcc/config/rs6000/rs6000.cc
>> @@ -10309,15 +10309,18 @@ can_be_rotated_to_negative_lis (HOST_WIDE_INT c,
>> int *rot)
>>
>> /* case b. xx0..01..1xx: some of 15 x's (and some of 16 0's) are
>> rotated over the highest bit. */
>> - int pos_one = clz_hwi ((c << 16) >> 16);
>> - middle_zeros = ctz_hwi (c >> (HOST_BITS_PER_WIDE_INT - pos_one));
>> - int middle_ones = clz_hwi (~(c << pos_one));
>> - if (middle_zeros >= 16 && middle_ones >= 33)
>> + unsigned HOST_WIDE_INT uc = (unsigned HOST_WIDE_INT)c;
> Space after cast:
> unsigned HOST_WIDE_INT uc = (unsigned HOST_WIDE_INT) c;
>
> (throughout).
>
>> + int pos_one = clz_hwi ((HOST_WIDE_INT)(uc << 16) >> 16);
> int pos_one = clz_hwi ((HOST_WIDE_INT) (uc << 16) >> 16);
>
> But why do you want a cast here? uc is already unsigned!
Cast is intentional, before performing the right shift, we cast uc back to
signed to preserve the sign bit properly during arithmetic shifts.
>> + if (pos_one != 0)
> Why this? Shifting by zero is perfectly fine.
>
>> {
>> - *rot = pos_one;
>> - return true;
>> + middle_zeros = ctz_hwi (c >> (HOST_BITS_PER_WIDE_INT - pos_one));
Concern is when pos_one is zero, then we end up doing c >> 64, which results in
undefined behavior.
>> @@ -10434,7 +10437,7 @@ can_be_built_by_li_and_rldic (HOST_WIDE_INT c, int
>> *shift, HOST_WIDE_INT *mask)
>> if (lz >= HOST_BITS_PER_WIDE_INT)
>> return false;
>>
>> - int middle_ones = clz_hwi (~(c << lz));
>> + int middle_ones = clz_hwi (~(((unsigned HOST_WIDE_INT)c) << lz));
>> if (tz + lz + middle_ones >= ones
>> && (tz - lz) < HOST_BITS_PER_WIDE_INT
>> && tz < HOST_BITS_PER_WIDE_INT)
>> @@ -10468,7 +10471,7 @@ can_be_built_by_li_and_rldic (HOST_WIDE_INT c, int
>> *shift, HOST_WIDE_INT *mask)
>> if (!IN_RANGE (pos_first_1, 1, HOST_BITS_PER_WIDE_INT-1))
>> return false;
>>
>> - middle_ones = clz_hwi (~c << pos_first_1);
>> + middle_ones = clz_hwi ((~(unsigned HOST_WIDE_INT)c) << pos_first_1);
>> middle_zeros = ctz_hwi (c >> (HOST_BITS_PER_WIDE_INT - pos_first_1));
>> if (pos_first_1 < HOST_BITS_PER_WIDE_INT
>> && middle_ones + middle_zeros < HOST_BITS_PER_WIDE_INT
>> @@ -10570,7 +10573,8 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT
>> c, int *num_insns)
>> {
>> /* li/lis; rldicX */
>> unsigned HOST_WIDE_INT imm = (c | ~mask);
>> - imm = (imm >> shift) | (imm << (HOST_BITS_PER_WIDE_INT - shift));
>> + if (shift != 0)
>> + imm = (imm >> shift) | (imm << (HOST_BITS_PER_WIDE_INT - shift));
> Same, why is this needed? The code is nicer without it.
Same here, if shift == 0, imm << 64 becomes undefined. Adding the check avoids
that.
>> count_or_emit_insn (temp, GEN_INT (imm));
>> if (shift != 0)
>> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
>> index 9c718ca2a22..8fc079a4297 100644
>> --- a/gcc/config/rs6000/rs6000.md
>> +++ b/gcc/config/rs6000/rs6000.md
>> @@ -1971,7 +1971,9 @@
>> {
>> HOST_WIDE_INT val = INTVAL (operands[2]);
>> HOST_WIDE_INT low = sext_hwi (val, 16);
>> - HOST_WIDE_INT rest = trunc_int_for_mode (val - low, <MODE>mode);
>> + /* Avoid signed overflow by computing difference in unsigned domain. */
>> + unsigned HOST_WIDE_INT urest = (unsigned HOST_WIDE_INT)val - (unsigned
>> HOST_WIDE_INT)low;
> You never need to cast both arms of a binary operator. C will promote
> one side automatically.
>
>> + HOST_WIDE_INT rest = trunc_int_for_mode (urest, <MODE>mode);
>>
>> operands[4] = GEN_INT (low);
>> if (<MODE>mode == SImode || satisfies_constraint_L (GEN_INT (rest)))
> So, it is probably best if you use somnething like
> unsigned HOST_WIDE_INT val = UINTVAL (operands[2]);
> at the start, and no other changes are needed?
Yes, works fine. I’ll update the patch accordingly and send out v2 with these
changes.
Kishan
