| Message ID | 56704D17.4030103@foss.arm.com |
|---|---|
| State | New |
| Headers | show |
On 12/15/2015 06:25 PM, Kyrill Tkachov wrote:
> Bootstrapped and tested on arm, aarch64, x86_64.
I'd say let's wait. Some of the changes look misindented btw.
Bernd
On 12/15/2015 10:25 AM, Kyrill Tkachov wrote: > Hi all, > > This converts the preprocessor check for WORD_REGISTER_OPERATIONS into a > runtime one > in rtlanal.c. > > Since this one was in combination with an "#if defined" and used to > guard an if-statement > I'd appreciate it if someone gave it a double-check that I dind't screw > up the intended > behaviour. > > Bootstrapped and tested on arm, aarch64, x86_64. > > Ok for trunk? > > Thanks, > Kyrill > > 2015-12-15 Kyrylo Tkachov <kyrylo.tkachov@arm.com> > > * rtlanal.c (nonzero_bits1): Convert preprocessor check > for WORD_REGISTER_OPERATIONS to runtime check. OK. jeff
> This converts the preprocessor check for WORD_REGISTER_OPERATIONS into a > runtime one in rtlanal.c. > > Since this one was in combination with an "#if defined" and used to guard an > if-statement I'd appreciate it if someone gave it a double-check that I > dind't screw up the intended behaviour. Unfortunately I think you did, as the old version was: #if WORD_REGISTER_OPERATIONS && defined (LOAD_EXTEND_OP) /* If this is a typical RISC machine, we only have to worry about the way loads are extended. */ if ((LOAD_EXTEND_OP (inner_mode) == SIGN_EXTEND ? val_signbit_known_set_p (inner_mode, nonzero) : LOAD_EXTEND_OP (inner_mode) != ZERO_EXTEND) || !MEM_P (SUBREG_REG (x))) #endif and the new version is: #ifdef LOAD_EXTEND_OP /* If this is a typical RISC machine, we only have to worry about the way loads are extended. */ if (WORD_REGISTER_OPERATIONS && ((LOAD_EXTEND_OP (inner_mode) == SIGN_EXTEND ? val_signbit_known_set_p (inner_mode, nonzero) : LOAD_EXTEND_OP (inner_mode) != ZERO_EXTEND) || !MEM_P (SUBREG_REG (x)))) #endif So if WORD_REGISTER_OPERATIONS is zero and LOAD_EXTEND_OP is defined, for example on PowerPC, the block guarded by the condition is always executed in the former case but never in the latter case.
Hi Eric, On 02/11/16 10:47, Eric Botcazou wrote: >> This converts the preprocessor check for WORD_REGISTER_OPERATIONS into a >> runtime one in rtlanal.c. >> >> Since this one was in combination with an "#if defined" and used to guard an >> if-statement I'd appreciate it if someone gave it a double-check that I >> dind't screw up the intended behaviour. > Unfortunately I think you did, as the old version was: > > #if WORD_REGISTER_OPERATIONS && defined (LOAD_EXTEND_OP) > /* If this is a typical RISC machine, we only have to worry > about the way loads are extended. */ > if ((LOAD_EXTEND_OP (inner_mode) == SIGN_EXTEND > ? val_signbit_known_set_p (inner_mode, nonzero) > : LOAD_EXTEND_OP (inner_mode) != ZERO_EXTEND) > || !MEM_P (SUBREG_REG (x))) > #endif > > and the new version is: > > #ifdef LOAD_EXTEND_OP > /* If this is a typical RISC machine, we only have to worry > about the way loads are extended. */ > if (WORD_REGISTER_OPERATIONS > && ((LOAD_EXTEND_OP (inner_mode) == SIGN_EXTEND > ? val_signbit_known_set_p (inner_mode, nonzero) > : LOAD_EXTEND_OP (inner_mode) != ZERO_EXTEND) > || !MEM_P (SUBREG_REG (x)))) > #endif > > So if WORD_REGISTER_OPERATIONS is zero and LOAD_EXTEND_OP is defined, for > example on PowerPC, the block guarded by the condition is always executed in > the former case but never in the latter case. I think you're right. I suppose the new condition should be: #ifdef LOAD_EXTEND_OP /* If this is a typical RISC machine, we only have to worry about the way loads are extended. */ if (!WORD_REGISTER_OPERATIONS || ((LOAD_EXTEND_OP (inner_mode) == SIGN_EXTEND ? val_signbit_known_set_p (inner_mode, nonzero) : LOAD_EXTEND_OP (inner_mode) != ZERO_EXTEND) || !MEM_P (SUBREG_REG (x)))) #endif Would you prefer me to make this change or just revert the patch? Thanks, Kyrill
> I think you're right. I suppose the new condition should be: > > #ifdef LOAD_EXTEND_OP > /* If this is a typical RISC machine, we only have to worry > about the way loads are extended. */ > if (!WORD_REGISTER_OPERATIONS > > || ((LOAD_EXTEND_OP (inner_mode) == SIGN_EXTEND > > ? val_signbit_known_set_p (inner_mode, nonzero) > > : LOAD_EXTEND_OP (inner_mode) != ZERO_EXTEND) > || > || !MEM_P (SUBREG_REG (x)))) > > #endif Agreed. > Would you prefer me to make this change or just revert the patch? Go ahead and make the change, but please do a bit of comment massaging in the process, for example: #ifdef LOAD_EXTEND_OP /* On many CISC machines, accessing an object in a wider mode causes the high-order bits to become undefined. So they are not known to be zero. */ if (!WORD_REGISTER_OPERATIONS /* If this is a typical RISC machine, we only have to worry about the way loads are extended. */ || ((LOAD_EXTEND_OP (inner_mode) == SIGN_EXTEND ? val_signbit_known_set_p (inner_mode, nonzero) : LOAD_EXTEND_OP (inner_mode) != ZERO_EXTEND) || !MEM_P (SUBREG_REG (x)))) #endif { if (GET_MODE_PRECISION (GET_MODE (x)) > GET_MODE_PRECISION (inner_mode)) nonzero |= (GET_MODE_MASK (GET_MODE (x)) & ~GET_MODE_MASK (inner_mode)); }
diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c index f893bca0d0a17498c1b234492e4acff02cec6e84..ab49602b72984e336f01a6b13f94993af9e4b8f9 100644 --- a/gcc/rtlanal.c +++ b/gcc/rtlanal.c @@ -4540,13 +4540,14 @@ nonzero_bits1 (const_rtx x, machine_mode mode, const_rtx known_x, nonzero &= cached_nonzero_bits (SUBREG_REG (x), mode, known_x, known_mode, known_ret); -#if WORD_REGISTER_OPERATIONS && defined (LOAD_EXTEND_OP) +#ifdef LOAD_EXTEND_OP /* If this is a typical RISC machine, we only have to worry about the way loads are extended. */ - if ((LOAD_EXTEND_OP (inner_mode) == SIGN_EXTEND - ? val_signbit_known_set_p (inner_mode, nonzero) - : LOAD_EXTEND_OP (inner_mode) != ZERO_EXTEND) - || !MEM_P (SUBREG_REG (x))) + if (WORD_REGISTER_OPERATIONS + && ((LOAD_EXTEND_OP (inner_mode) == SIGN_EXTEND + ? val_signbit_known_set_p (inner_mode, nonzero) + : LOAD_EXTEND_OP (inner_mode) != ZERO_EXTEND) + || !MEM_P (SUBREG_REG (x)))) #endif { /* On many CISC machines, accessing an object in a wider mode
Hi all, This converts the preprocessor check for WORD_REGISTER_OPERATIONS into a runtime one in rtlanal.c. Since this one was in combination with an "#if defined" and used to guard an if-statement I'd appreciate it if someone gave it a double-check that I dind't screw up the intended behaviour. Bootstrapped and tested on arm, aarch64, x86_64. Ok for trunk? Thanks, Kyrill 2015-12-15 Kyrylo Tkachov <kyrylo.tkachov@arm.com> * rtlanal.c (nonzero_bits1): Convert preprocessor check for WORD_REGISTER_OPERATIONS to runtime check.