On 11/29/24 1:43 AM, Jakub Jelinek wrote:
Hi!
This is mostly a blind attempt to fix the PR + various cleanups.
The PR is about a shift of a HOST_WIDE_INT by 127 invoking UB.
Most of carry_backpropagate works on GET_MODE_INNER of the operand,
mode is assigned
enum machine_mode mode = GET_MODE_INNER (GET_MODE (x));
at the beginning and everything is done using that mode, so for
vector modes (or complex even?) we work with the element modes
rather than vector/complex modes.
But the SIGN_EXTEND handling does that inconsistently, it looks
at mode of the operand and uses GET_MODE_INNER in GET_MODE_MASK,
but doesn't use it in the shift.
The following patch appart from the cleanups fixes it by doing
essentially:
mode = GET_MODE (XEXP (x, 0));
if (mask & ~GET_MODE_MASK (GET_MODE_INNER (mode)))
- mask |= 1ULL << (GET_MODE_BITSIZE (mode).to_constant () - 1);
+ mask |= 1ULL << (GET_MODE_BITSIZE (GET_MODE_INNER (mode)).to_constant
() - 1);
i.e. also shifting by GET_MODE_BITSIZE of the GET_MODE_INNER of the
operand's mode. We don't need to check if it is at most 64 bits,
at the start of the function we've already verified the result mode
is at most 64 bits and SIGN_EXTEND by definition extends from a narrower
mode.
Yea, the code was definitely not consistent in this regard. Thanks for
cleaning it up.
The rest of the patch are cleanups. For HOST_WIDE_INT we have the
HOST_WIDE_INT_{UC,1U} macros, a HWI isn't necessarily unsigned long long,
so using ULL suffixes for it is weird.
Yea, the code clearly needed a lot of cleanups related to its constants.
I actually just cleaned up several similar problems in the CRC bits I
committed earlier this week. It's an easy issue to miss.
More importantly, the function does
scalar_int_mode smode;
if (!is_a <scalar_int_mode> (mode, &smode)
|| GET_MODE_BITSIZE (smode) > HOST_BITS_PER_WIDE_INT)
return mmask;
early, so we don't need to use GET_MODE_BITSIZE (mode) which is
a poly_int but can use GET_MODE_BITSIZE (smode) with the same value
but in unsigned short, so we don't need to use known_lt or .to_constant ()
everywhere.
Good cleanup. I'd started working backwards from the bottom of the
patch and had repeatedly asked myself if the changes were safe. This
explains it just fine.
Plus some formatting issues.
What I've left around is
if (!GET_MODE_BITSIZE (GET_MODE (x)).is_constant ()
|| !GET_MODE_BITSIZE (GET_MODE (XEXP (x, 0))).is_constant ())
return -1;
at the start of SIGN_EXTEND or ZERO_EXTEND, I'm afraid I don't know enough
about aarch64/riscv VL vectors to know why this is done (though even that
return -1; is weird, rest of the code does return mmask; if it wants to
punt.
It's just saying everything is live if we're presented with a mode that
doesn't have a constant side. mmask, if wide enough, should work
better in theory.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2024-11-29 Jakub Jelinek <ja...@redhat.com>
PR rtl-optimization/117360
* ext-dce.cc (ext_dce_process_sets): Use HOST_WIDE_INT_UC
macro instead of ULL suffixed constants.
(carry_backpropagate): Likewise. Use HOST_WIDE_INT_1U instead of
1ULL. Use GET_MODE_BITSIZE (smode) instead of
GET_MODE_BITSIZE (mode) and with that avoid having to use
known_lt instead of < or use .to_constant (). Formatting fixes.
(case SIGN_EXTEND): Set mode to GET_MODE_INNER (GET_MODE (XEXP (x, 0)))
rather than GET_MODE (XEXP (x, 0)) and don't use GET_MODE_INNER (mode).
(ext_dce_process_uses): Use HOST_WIDE_INT_UC macro instead of ULL
suffixed constants.
Thanks. I hadn't forgotten this, but was still a few days away from
being able to focus on it.
I still have other concerns about this code (the carry_backpropagate
bits), but your changes don't impact those concerns at all.
OK for the trunk.
jeff
