| Message ID | 5411A31E.3030207@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
On Thu, Sep 11, 2014 at 6:36 PM, Bernd Schmidt <bernds@codesourcery.com> 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 > >
* 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)))
{