Hi!
On Wed, Jul 30, 2025 at 01:12:27PM +0530, Kishan Parmar wrote:
> PR target/118890
> * config/rs6000/rs6000.cc (can_be_rotated_to_negative_lis): Avoid left
> shift of
Line too long, changelog lines are only 80 character positions max.
> negative value and guard shift count.
> (can_be_built_by_li_and_rldic): Likewise.
> (rs6000_emit_set_long_const): Likewise.
> * config/rs6000/rs6000.md (splitter for plus into two 16-bit parts):
> Fix error.
Same.
"Fix error" is a pretty lousy description, of course. What was wrong,
what do we do instead now?
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -10320,15 +10320,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;
> + int pos_one = clz_hwi ((HOST_WIDE_INT) (uc << 16) >> 16);
Do you need a cast here? Just assigning should work fine?
Ideally, you will only ever have a cast if that changes the value! And
everything else can be done by using appropriate data types in the first
place. For the reader of the code it then is obvious you are *not*
changing the value.
Very very VERY simply said: whenever you need to look deeply to see if
a cast is correct, it probably either is not, or it is superfluous.
> - int middle_ones = clz_hwi (~(c << lz));
> + int middle_ones = clz_hwi (~(((unsigned HOST_WIDE_INT) c) << lz));
Same here.
unsigned HOST_WIDE_INT uc = c;
int middle_ones = clz_hwi (~(uc << lz));
or something like that. Simpler, more readable, and potentially even
correct (unlike the original!)
> {
> /* 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));
So there were good examples already! Why change this though? Ah, the
rightmost part can shift by a register worth of bits, which isn't valid
C, although it would be perfectly fine machine code for us. Yeah ok.
Okay for trunk with those things fixed. Thank you!
Segher
