Re: ptx preliminary rtl patches [3/4]
On Thu, Sep 18, 2014 at 8:57 PM, Bernd Schmidt wrote: > On 09/12/2014 10:04 AM, Richard Biener wrote: >> >> On Thu, Sep 11, 2014 at 6:36 PM, Bernd Schmidt >> wrote: >>> >>> I strongly disagree. It's the same as for any other integer - there's one >>> sign bit, and since there aren't any other bits, the number of sign bit >>> copies is always exactly 1. >> >> >> I agree about that. But I fail to see what goes wrong with the existing >> code in combine. Maybe the code simply doesn't work for >> GET_MODE_PRECISION != GET_MODE_BITSIZE? > > > I had to debug it again - the patch was a year old. This time I came to the > conclusion that we're just using the wrong mode. We're trying to simplify > (ne:BI (reg:HI x) (const_int 0)), and the code here was using BImode when > calling num_sign_bit_copies for the register - what I think it wants to do > is verify that the operand consists of all zeros or all ones. > > Digging a bit further I noticed that some of the cases around this code have > a mode == GET_MODE (op0) test. These were added by rth in commit 3573fd048, > which added BImode. It looks like this particular case slipped through the > cracks. The easiest way to fix it is the below - bootstrapped and tested on > x86_64-linux, ok if it also works with ptx? Ok. Thanks, Richard. > > Bernd >
Re: ptx preliminary rtl patches [3/4]
On 09/12/2014 10:04 AM, Richard Biener wrote: On Thu, Sep 11, 2014 at 6:36 PM, Bernd Schmidt wrote: I strongly disagree. It's the same as for any other integer - there's one sign bit, and since there aren't any other bits, the number of sign bit copies is always exactly 1. I agree about that. But I fail to see what goes wrong with the existing code in combine. Maybe the code simply doesn't work for GET_MODE_PRECISION != GET_MODE_BITSIZE? I had to debug it again - the patch was a year old. This time I came to the conclusion that we're just using the wrong mode. We're trying to simplify (ne:BI (reg:HI x) (const_int 0)), and the code here was using BImode when calling num_sign_bit_copies for the register - what I think it wants to do is verify that the operand consists of all zeros or all ones. Digging a bit further I noticed that some of the cases around this code have a mode == GET_MODE (op0) test. These were added by rth in commit 3573fd048, which added BImode. It looks like this particular case slipped through the cracks. The easiest way to fix it is the below - bootstrapped and tested on x86_64-linux, ok if it also works with ptx? Bernd commit 3e1c88a4a16c768e11fd56b881ed577fba33 Author: Bernd Schmidt Date: Tue Sep 16 21:04:55 2014 +0200 * combine.c (combine_simplify_rtx): In STORE_FLAG_VALUE == -1 case, also verify that mode is equal to the mode of op0. diff --git a/gcc/combine.c b/gcc/combine.c index 04863cb..efdd638 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -5801,10 +5801,11 @@ combine_simplify_rtx (rtx x, enum machine_mode op0_mode, int in_dest, ; else if (STORE_FLAG_VALUE == -1 - && new_code == NE && GET_MODE_CLASS (mode) == MODE_INT - && op1 == const0_rtx - && (num_sign_bit_copies (op0, mode) - == GET_MODE_PRECISION (mode))) + && new_code == NE && GET_MODE_CLASS (mode) == MODE_INT + && op1 == const0_rtx + && mode == GET_MODE (op0) + && (num_sign_bit_copies (op0, mode) + == GET_MODE_PRECISION (mode))) return gen_lowpart (mode, expand_compound_operation (op0));
Re: ptx preliminary rtl patches [3/4]
On Thu, Sep 11, 2014 at 6:36 PM, Bernd Schmidt wrote: > On 09/11/2014 06:34 PM, Steven Bosscher wrote: >> >> On Thu, Sep 11, 2014 at 6:19 PM, Bernd Schmidt wrote: > > What do you expect that function to do different? It returns the > correct > value. > No different. Just that if you want to check whether DECL is a global variable then we have a predicate for it. So why use TREE_STATIC instead? In other words: Just trying to make/keep certain checks consistent. (A hopeless cause, but a noble one... ;-) >>> >>> >>> >>> You're talking about a different patch here. This one is about >>> num_sign_bit_copies. >> >> >> >> Ah. *sigh* can't even keep two patches in my mind at any one time. >> >> The point about num_sign_bit_copies is that it doesn't really return >> the correct value IMHO, if there isn't really a correct value to speak >> of: What is the sign of TRUE or FALSE, the only two values a BImode >> value can take? >> >> A 1-bit precision integer can have value 0 or -1 and in that case >> num_sign_bit_copies should be 0. But for a BImode value, it seems to >> me that asking for the sign bit or sign bit copies is just wrong. > > > I strongly disagree. It's the same as for any other integer - there's one > sign bit, and since there aren't any other bits, the number of sign bit > copies is always exactly 1. I agree about that. But I fail to see what goes wrong with the existing code in combine. Maybe the code simply doesn't work for GET_MODE_PRECISION != GET_MODE_BITSIZE? At least your new comment doesn't explain anything to me. Richard. > > Bernd > >
Re: ptx preliminary rtl patches [3/4]
On 09/11/2014 06:34 PM, Steven Bosscher wrote: On Thu, Sep 11, 2014 at 6:19 PM, Bernd Schmidt wrote: What do you expect that function to do different? It returns the correct value. No different. Just that if you want to check whether DECL is a global variable then we have a predicate for it. So why use TREE_STATIC instead? In other words: Just trying to make/keep certain checks consistent. (A hopeless cause, but a noble one... ;-) You're talking about a different patch here. This one is about num_sign_bit_copies. Ah. *sigh* can't even keep two patches in my mind at any one time. The point about num_sign_bit_copies is that it doesn't really return the correct value IMHO, if there isn't really a correct value to speak of: What is the sign of TRUE or FALSE, the only two values a BImode value can take? A 1-bit precision integer can have value 0 or -1 and in that case num_sign_bit_copies should be 0. But for a BImode value, it seems to me that asking for the sign bit or sign bit copies is just wrong. I strongly disagree. It's the same as for any other integer - there's one sign bit, and since there aren't any other bits, the number of sign bit copies is always exactly 1. Bernd
Re: ptx preliminary rtl patches [3/4]
On Thu, Sep 11, 2014 at 6:19 PM, Bernd Schmidt wrote: >>> What do you expect that function to do different? It returns the correct >>> value. >>> >> >> No different. Just that if you want to check whether DECL is a global >> variable then we have a predicate for it. So why use TREE_STATIC >> instead? >> >> In other words: Just trying to make/keep certain checks consistent. (A >> hopeless cause, but a noble one... ;-) > > > You're talking about a different patch here. This one is about > num_sign_bit_copies. Ah. *sigh* can't even keep two patches in my mind at any one time. The point about num_sign_bit_copies is that it doesn't really return the correct value IMHO, if there isn't really a correct value to speak of: What is the sign of TRUE or FALSE, the only two values a BImode value can take? A 1-bit precision integer can have value 0 or -1 and in that case num_sign_bit_copies should be 0. But for a BImode value, it seems to me that asking for the sign bit or sign bit copies is just wrong. Ciao! Steven
Re: ptx preliminary rtl patches [3/4]
On 09/11/2014 06:15 PM, Steven Bosscher wrote: On Thu, Sep 11, 2014 at 6:06 PM, Bernd Schmidt wrote: On 09/11/2014 05:55 PM, Steven Bosscher wrote: On Thu, Sep 11, 2014 at 3:26 PM, Bernd Schmidt wrote: nvptx will be the first port to use BImode and have STORE_FLAG_VALUE==-1. That has exposed a bug in combine where we can end up calling num_sign_bit_copies for a BImode value. However, the return value is always 1 in that case, so it doesn't tell us anything and is going to be misinterpreted by the caller. Bootstrapped and tested on x86_64-linux, together with the other patches. Ok? This should be handled in num_sign_bit_copies itself, i.e. handle BImode there. What do you expect that function to do different? It returns the correct value. No different. Just that if you want to check whether DECL is a global variable then we have a predicate for it. So why use TREE_STATIC instead? In other words: Just trying to make/keep certain checks consistent. (A hopeless cause, but a noble one... ;-) You're talking about a different patch here. This one is about num_sign_bit_copies. I can certainly use is_global_var if the patch is ok with that change. Bernd
Re: ptx preliminary rtl patches [3/4]
On Thu, Sep 11, 2014 at 6:06 PM, Bernd Schmidt wrote: > On 09/11/2014 05:55 PM, Steven Bosscher wrote: >> >> On Thu, Sep 11, 2014 at 3:26 PM, Bernd Schmidt wrote: >>> >>> nvptx will be the first port to use BImode and have STORE_FLAG_VALUE==-1. >>> That has exposed a bug in combine where we can end up calling >>> num_sign_bit_copies for a BImode value. However, the return value is >>> always >>> 1 in that case, so it doesn't tell us anything and is going to be >>> misinterpreted by the caller. >>> >>> Bootstrapped and tested on x86_64-linux, together with the other patches. >>> Ok? >> >> >> This should be handled in num_sign_bit_copies itself, i.e. handle BImode >> there. > > > What do you expect that function to do different? It returns the correct > value. > No different. Just that if you want to check whether DECL is a global variable then we have a predicate for it. So why use TREE_STATIC instead? In other words: Just trying to make/keep certain checks consistent. (A hopeless cause, but a noble one... ;-) Ciao! Steven
Re: ptx preliminary rtl patches [3/4]
On 09/11/2014 05:55 PM, Steven Bosscher wrote: On Thu, Sep 11, 2014 at 3:26 PM, Bernd Schmidt wrote: nvptx will be the first port to use BImode and have STORE_FLAG_VALUE==-1. That has exposed a bug in combine where we can end up calling num_sign_bit_copies for a BImode value. However, the return value is always 1 in that case, so it doesn't tell us anything and is going to be misinterpreted by the caller. Bootstrapped and tested on x86_64-linux, together with the other patches. Ok? This should be handled in num_sign_bit_copies itself, i.e. handle BImode there. What do you expect that function to do different? It returns the correct value. Bernd
Re: ptx preliminary rtl patches [3/4]
On Thu, Sep 11, 2014 at 3:26 PM, Bernd Schmidt wrote: > nvptx will be the first port to use BImode and have STORE_FLAG_VALUE==-1. > That has exposed a bug in combine where we can end up calling > num_sign_bit_copies for a BImode value. However, the return value is always > 1 in that case, so it doesn't tell us anything and is going to be > misinterpreted by the caller. > > Bootstrapped and tested on x86_64-linux, together with the other patches. > Ok? This should be handled in num_sign_bit_copies itself, i.e. handle BImode there. Ciao! Steven
ptx preliminary rtl patches [3/4]
nvptx will be the first port to use BImode and have
STORE_FLAG_VALUE==-1. That has exposed a bug in combine where we can end
up calling num_sign_bit_copies for a BImode value. However, the return
value is always 1 in that case, so it doesn't tell us anything and is
going to be misinterpreted by the caller.
Bootstrapped and tested on x86_64-linux, together with the other
patches. Ok?
Bernd
* combine.c (combine_simplify_rtx): Avoid using num_sign_bit_copies
for single-bit modes.
diff --git a/gcc/combine.c b/gcc/combine.c
index fe95b41..49c6baa 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -5801,10 +5801,14 @@ combine_simplify_rtx (rtx x, enum machine_mode op0_mode, int in_dest,
;
else if (STORE_FLAG_VALUE == -1
- && new_code == NE && GET_MODE_CLASS (mode) == MODE_INT
- && op1 == const0_rtx
- && (num_sign_bit_copies (op0, mode)
- == GET_MODE_PRECISION (mode)))
+ && new_code == NE && GET_MODE_CLASS (mode) == MODE_INT
+ && op1 == const0_rtx
+ /* There's always at least one sign bit copy in a
+ one-bit mode, so the call to num_sign_bit_copies
+ tells us nothing in that case. */
+ && GET_MODE_PRECISION (mode) > 1
+ && (num_sign_bit_copies (op0, mode)
+ == GET_MODE_PRECISION (mode)))
return gen_lowpart (mode,
expand_compound_operation (op0));
@@ -5824,6 +5828,7 @@ combine_simplify_rtx (rtx x, enum machine_mode op0_mode, int in_dest,
&& new_code == EQ && GET_MODE_CLASS (mode) == MODE_INT
&& op1 == const0_rtx
&& mode == GET_MODE (op0)
+ && GET_MODE_PRECISION (mode) > 1
&& (num_sign_bit_copies (op0, mode)
== GET_MODE_PRECISION (mode)))
{
