| Message ID | ffb1b7f7e4414214d2835fb534dcc414445f5e03.1486654190.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
Segher Boessenkool <segher@kernel.crashing.org> writes: > As stated in the PR, this test now passes on aarch64, ia64, powerpc, > and s390x. This patch disables the xfails for those targets. As I'd mentioned in PR tree-optimization/78775, the test XPASSes on SPARC, too. > Tested on powerpc64-linux {-m32,-m64}. Is this okay for trunk? [...] > 2017-02-09 Segher Boessenkool <segher@kernel.crashing.org> > > gcc/testsuite/ > PR testsuite/79356 > * gcc.dg/attr-alloc_size-11.c: Don't xfail on aarch64, ia64, powerpc, > or s390x. TBH, I'd strongly prefer to have a proper analysis instead of just un-xfail-ing the test on an ever growing apparently random list of targets. Rainer
On Fri, Feb 10, 2017 at 11:56:39AM +0100, Rainer Orth wrote: > Segher Boessenkool <segher@kernel.crashing.org> writes: > > > As stated in the PR, this test now passes on aarch64, ia64, powerpc, > > and s390x. This patch disables the xfails for those targets. > > As I'd mentioned in PR tree-optimization/78775, the test XPASSes on > SPARC, too. > > > Tested on powerpc64-linux {-m32,-m64}. Is this okay for trunk? > [...] > > 2017-02-09 Segher Boessenkool <segher@kernel.crashing.org> > > > > gcc/testsuite/ > > PR testsuite/79356 > > * gcc.dg/attr-alloc_size-11.c: Don't xfail on aarch64, ia64, powerpc, > > or s390x. > > TBH, I'd strongly prefer to have a proper analysis instead of just > un-xfail-ing the test on an ever growing apparently random list of > targets. Yeah, I agree. I thought it was just another test that stopped failing, but it seems to fail in two ways now, making the testcase succeed? Lovely. Please consider this patch withdrawn. Segher
Hi Segher, > On Fri, Feb 10, 2017 at 11:56:39AM +0100, Rainer Orth wrote: >> Segher Boessenkool <segher@kernel.crashing.org> writes: >> >> > As stated in the PR, this test now passes on aarch64, ia64, powerpc, >> > and s390x. This patch disables the xfails for those targets. >> >> As I'd mentioned in PR tree-optimization/78775, the test XPASSes on >> SPARC, too. >> >> > Tested on powerpc64-linux {-m32,-m64}. Is this okay for trunk? >> [...] >> > 2017-02-09 Segher Boessenkool <segher@kernel.crashing.org> >> > >> > gcc/testsuite/ >> > PR testsuite/79356 >> > * gcc.dg/attr-alloc_size-11.c: Don't xfail on aarch64, ia64, powerpc, >> > or s390x. >> >> TBH, I'd strongly prefer to have a proper analysis instead of just >> un-xfail-ing the test on an ever growing apparently random list of >> targets. > > Yeah, I agree. I thought it was just another test that stopped failing, > but it seems to fail in two ways now, making the testcase succeed? > Lovely. Please consider this patch withdrawn. I just noticed that nothing has happened at all in a month, so anything is better than the tests XPASSing on a number of targets. So the patch is ok for mainline with sparc*-*-* added to the target lists and a reference to PR testsuite/79356 in the comment. I'd still be very grateful if Martin could have a look what's really going on here, though. Thanks. Rainer
> > I just noticed that nothing has happened at all in a month, so anything > is better than the tests XPASSing on a number of targets. > > So the patch is ok for mainline with sparc*-*-* added to the target > lists and a reference to PR testsuite/79356 in the comment. > > I'd still be very grateful if Martin could have a look what's really > going on here, though. > > Thanks. > Rainer > Hi, This test has also been XPASS-ing for mips-mti-elf and mips-mti-linux-gnu, so I think you could also add mips*-*-* to the xfail targets. Regards, Toma
Hi Toma, >> I just noticed that nothing has happened at all in a month, so anything >> is better than the tests XPASSing on a number of targets. >> >> So the patch is ok for mainline with sparc*-*-* added to the target >> lists and a reference to PR testsuite/79356 in the comment. >> >> I'd still be very grateful if Martin could have a look what's really >> going on here, though. > This test has also been XPASS-ing for mips-mti-elf and mips-mti-linux-gnu, > so I think you could also add mips*-*-* to the xfail targets. no argument from me. This just underlines that someone needs to investigate what's really going on here. Consider such a patch pre-approved. Rainer
On Fri, Mar 10, 2017 at 03:31:39PM +0100, Rainer Orth wrote: > Hi Toma, > > >> I just noticed that nothing has happened at all in a month, so anything > >> is better than the tests XPASSing on a number of targets. > >> > >> So the patch is ok for mainline with sparc*-*-* added to the target > >> lists and a reference to PR testsuite/79356 in the comment. > >> > >> I'd still be very grateful if Martin could have a look what's really > >> going on here, though. > > > This test has also been XPASS-ing for mips-mti-elf and mips-mti-linux-gnu, > > so I think you could also add mips*-*-* to the xfail targets. > > no argument from me. This just underlines that someone needs to > investigate what's really going on here. > > Consider such a patch pre-approved. gcc-testresults shows it passing on some other mips targets as well. I'll include it. Segher
On 03/10/2017 05:57 AM, Rainer Orth wrote: > Hi Segher, > >> On Fri, Feb 10, 2017 at 11:56:39AM +0100, Rainer Orth wrote: >>> Segher Boessenkool <segher@kernel.crashing.org> writes: >>> >>>> As stated in the PR, this test now passes on aarch64, ia64, powerpc, >>>> and s390x. This patch disables the xfails for those targets. >>> >>> As I'd mentioned in PR tree-optimization/78775, the test XPASSes on >>> SPARC, too. >>> >>>> Tested on powerpc64-linux {-m32,-m64}. Is this okay for trunk? >>> [...] >>>> 2017-02-09 Segher Boessenkool <segher@kernel.crashing.org> >>>> >>>> gcc/testsuite/ >>>> PR testsuite/79356 >>>> * gcc.dg/attr-alloc_size-11.c: Don't xfail on aarch64, ia64, powerpc, >>>> or s390x. >>> >>> TBH, I'd strongly prefer to have a proper analysis instead of just >>> un-xfail-ing the test on an ever growing apparently random list of >>> targets. >> >> Yeah, I agree. I thought it was just another test that stopped failing, >> but it seems to fail in two ways now, making the testcase succeed? >> Lovely. Please consider this patch withdrawn. > > I just noticed that nothing has happened at all in a month, so anything > is better than the tests XPASSing on a number of targets. > > So the patch is ok for mainline with sparc*-*-* added to the target > lists and a reference to PR testsuite/79356 in the comment. > > I'd still be very grateful if Martin could have a look what's really > going on here, though. Sorry, I haven't had a chance to look more deeply into why these assertions pass on some targets and fail on others. There is at least one bug that tracks a VRP problem that manifests only on some targets and not others (79054). I don't know if this is a symptom of the same bug or something different. I'll see if I can find some time to isolate it. Martin
On 03/10/2017 09:20 AM, Martin Sebor wrote: > On 03/10/2017 05:57 AM, Rainer Orth wrote: >> Hi Segher, >> >>> On Fri, Feb 10, 2017 at 11:56:39AM +0100, Rainer Orth wrote: >>>> Segher Boessenkool <segher@kernel.crashing.org> writes: >>>> >>>>> As stated in the PR, this test now passes on aarch64, ia64, powerpc, >>>>> and s390x. This patch disables the xfails for those targets. >>>> >>>> As I'd mentioned in PR tree-optimization/78775, the test XPASSes on >>>> SPARC, too. >>>> >>>>> Tested on powerpc64-linux {-m32,-m64}. Is this okay for trunk? >>>> [...] >>>>> 2017-02-09 Segher Boessenkool <segher@kernel.crashing.org> >>>>> >>>>> gcc/testsuite/ >>>>> PR testsuite/79356 >>>>> * gcc.dg/attr-alloc_size-11.c: Don't xfail on aarch64, ia64, >>>>> powerpc, >>>>> or s390x. >>>> >>>> TBH, I'd strongly prefer to have a proper analysis instead of just >>>> un-xfail-ing the test on an ever growing apparently random list of >>>> targets. >>> >>> Yeah, I agree. I thought it was just another test that stopped failing, >>> but it seems to fail in two ways now, making the testcase succeed? >>> Lovely. Please consider this patch withdrawn. >> >> I just noticed that nothing has happened at all in a month, so anything >> is better than the tests XPASSing on a number of targets. >> >> So the patch is ok for mainline with sparc*-*-* added to the target >> lists and a reference to PR testsuite/79356 in the comment. >> >> I'd still be very grateful if Martin could have a look what's really >> going on here, though. > > Sorry, I haven't had a chance to look more deeply into why these > assertions pass on some targets and fail on others. There is at > least one bug that tracks a VRP problem that manifests only on > some targets and not others (79054). I don't know if this is > a symptom of the same bug or something different. I'll see if > I can find some time to isolate it. It could well be a BRANCH_COST effect. BRANCH_COST is probably the most annoying target property that bleeds into the tree/gimple world. From looking at the gimple in the BZ that could well be the case. See logical_op_short_circuit for how this is often dealt with in the testsuite. jeff
On 03/10/2017 10:51 PM, Jeff Law wrote: > On 03/10/2017 09:20 AM, Martin Sebor wrote: >> On 03/10/2017 05:57 AM, Rainer Orth wrote: >>> Hi Segher, >>> >>>> On Fri, Feb 10, 2017 at 11:56:39AM +0100, Rainer Orth wrote: >>>>> Segher Boessenkool <segher@kernel.crashing.org> writes: >>>>> >>>>>> As stated in the PR, this test now passes on aarch64, ia64, powerpc, >>>>>> and s390x. This patch disables the xfails for those targets. >>>>> >>>>> As I'd mentioned in PR tree-optimization/78775, the test XPASSes on >>>>> SPARC, too. >>>>> >>>>>> Tested on powerpc64-linux {-m32,-m64}. Is this okay for trunk? >>>>> [...] >>>>>> 2017-02-09 Segher Boessenkool <segher@kernel.crashing.org> >>>>>> >>>>>> gcc/testsuite/ >>>>>> PR testsuite/79356 >>>>>> * gcc.dg/attr-alloc_size-11.c: Don't xfail on aarch64, ia64, >>>>>> powerpc, >>>>>> or s390x. >>>>> >>>>> TBH, I'd strongly prefer to have a proper analysis instead of just >>>>> un-xfail-ing the test on an ever growing apparently random list of >>>>> targets. >>>> >>>> Yeah, I agree. I thought it was just another test that stopped >>>> failing, >>>> but it seems to fail in two ways now, making the testcase succeed? >>>> Lovely. Please consider this patch withdrawn. >>> >>> I just noticed that nothing has happened at all in a month, so anything >>> is better than the tests XPASSing on a number of targets. >>> >>> So the patch is ok for mainline with sparc*-*-* added to the target >>> lists and a reference to PR testsuite/79356 in the comment. >>> >>> I'd still be very grateful if Martin could have a look what's really >>> going on here, though. >> >> Sorry, I haven't had a chance to look more deeply into why these >> assertions pass on some targets and fail on others. There is at >> least one bug that tracks a VRP problem that manifests only on >> some targets and not others (79054). I don't know if this is >> a symptom of the same bug or something different. I'll see if >> I can find some time to isolate it. > It could well be a BRANCH_COST effect. BRANCH_COST is probably the most > annoying target property that bleeds into the tree/gimple world. From > looking at the gimple in the BZ that could well be the case. > > See logical_op_short_circuit for how this is often dealt with in the > testsuite. Thanks for the hint. I extracted the test case from attr-alloc_size-11.c and reproduced the discrepancy on powerpc64le and x86_64. It looks to me like two bugs(?) conspire to trigger it. I opened pr80006 with the details and I'm working on a patch. Here's some more detail. First, on x86_64 (but not on powerpc64le and I suspect the other targets where this assertion passes), GCC very early on introduces a spurious conversion from signed char to int. For instance, given: void* f (signed char x) { extern void* ff (signed char) __attribute__ ((alloc_size (1))); if (-3 <= x && x <= 7) x = -4; return ff (x); } the tree-original dump on x86_64 is as follows (note the (int)x cast): ;; Function f (null) ;; enabled by -tree-original { extern void * ff (signed char); if ((unsigned char) x + 3 <= 10) { x = -4; } return ff ((int) x); <<< spurious cast } while the following the same dump on powerpc64le (no cast): ;; Function f (null) ;; enabled by -tree-original { extern void * ff (signed char); if ((unsigned char) x + 3 <= 10) { x = -4; } return ff (x); <<< no cast } The cast makes its way to VRP where it causes the range on x to be lost in the conversion to the temporary it introduces: Found new range for x_4: ~[-3, 7] marking stmt to be not simulated again Visiting statement: _9 = (int) x_4; Meeting [-128, -4] and [8, 127] to [-128, 127] Found new range for _9: [-128, 127] <<< anti-range lost ... Visiting PHI node: prephitmp_10 = PHI <_9(3), -4(2)> ... Meeting [-128, 127] and [-4, -4] to [-128, 127] Intersecting [-128, 127] and [-128, 127] to [-128, 127] Found new range for prephitmp_10: [-128, 127] ... Visiting statement: _8 = ff (prephitmp_10); Debugging tree-vrp.c suggests that this is a deficiency in the extract_range_from_unary_expr function the computes the range of the result of the NOP_EXPR (the cast) of an SSA_NAME with an anti-range ~[A, B] as a union of [TYPE_MIN, A - 1] and [B + 1, TYPE_MAX], which for ~[-3, 7] and the signed char x in the test case results in [SCHAR_MIN, -4] U [8, SCHAR_MAX], or [SCHAR_MIN, SCHAR_MAX]. There's a comment in the function that reads: /* Now canonicalize anti-ranges to ranges when they are not symbolic and express op ~[] as (op []') U (op []''). */ indicating this is deliberate but I have no idea why or if it's important. After adding code to handle this case none of the VRP tests broke so maybe it's just an oversight. But even with this case handled and the anti-range propagated to the result of the cast, there's still a problem. The vrp_visit_phi_node function that updates the range of the result of a PHI node merges the existing range of the LHS with the range of the PHI. This again results in the anti-range being lost and replaced with that of the type of the RHS (signed char here). Removing the merging the prephitmp_10 result of the cast has the same anti-range as the original and the warning in the test goes away even on x86_64. I have to do more testing to make sure these changes don't break anything. I am yet to look into the code that's responsible for the spurious cast to begin with. That should be removed independent of the VRP bug(s). Martin
On 03/12/2017 07:32 PM, Martin Sebor wrote: > On 03/10/2017 10:51 PM, Jeff Law wrote: >> On 03/10/2017 09:20 AM, Martin Sebor wrote: >>> On 03/10/2017 05:57 AM, Rainer Orth wrote: >>>> Hi Segher, >>>> >>>>> On Fri, Feb 10, 2017 at 11:56:39AM +0100, Rainer Orth wrote: >>>>>> Segher Boessenkool <segher@kernel.crashing.org> writes: >>>>>> >>>>>>> As stated in the PR, this test now passes on aarch64, ia64, powerpc, >>>>>>> and s390x. This patch disables the xfails for those targets. >>>>>> >>>>>> As I'd mentioned in PR tree-optimization/78775, the test XPASSes on >>>>>> SPARC, too. >>>>>> >>>>>>> Tested on powerpc64-linux {-m32,-m64}. Is this okay for trunk? >>>>>> [...] >>>>>>> 2017-02-09 Segher Boessenkool <segher@kernel.crashing.org> >>>>>>> >>>>>>> gcc/testsuite/ >>>>>>> PR testsuite/79356 >>>>>>> * gcc.dg/attr-alloc_size-11.c: Don't xfail on aarch64, ia64, >>>>>>> powerpc, >>>>>>> or s390x. >>>>>> >>>>>> TBH, I'd strongly prefer to have a proper analysis instead of just >>>>>> un-xfail-ing the test on an ever growing apparently random list of >>>>>> targets. >>>>> >>>>> Yeah, I agree. I thought it was just another test that stopped >>>>> failing, >>>>> but it seems to fail in two ways now, making the testcase succeed? >>>>> Lovely. Please consider this patch withdrawn. >>>> >>>> I just noticed that nothing has happened at all in a month, so anything >>>> is better than the tests XPASSing on a number of targets. >>>> >>>> So the patch is ok for mainline with sparc*-*-* added to the target >>>> lists and a reference to PR testsuite/79356 in the comment. >>>> >>>> I'd still be very grateful if Martin could have a look what's really >>>> going on here, though. >>> >>> Sorry, I haven't had a chance to look more deeply into why these >>> assertions pass on some targets and fail on others. There is at >>> least one bug that tracks a VRP problem that manifests only on >>> some targets and not others (79054). I don't know if this is >>> a symptom of the same bug or something different. I'll see if >>> I can find some time to isolate it. >> It could well be a BRANCH_COST effect. BRANCH_COST is probably the most >> annoying target property that bleeds into the tree/gimple world. From >> looking at the gimple in the BZ that could well be the case. >> >> See logical_op_short_circuit for how this is often dealt with in the >> testsuite. > > Thanks for the hint. I extracted the test case from > attr-alloc_size-11.c and reproduced the discrepancy on powerpc64le > and x86_64. It looks to me like two bugs(?) conspire to trigger > it. I opened pr80006 with the details and I'm working on a patch. > > Here's some more detail. First, on x86_64 (but not on powerpc64le > and I suspect the other targets where this assertion passes), GCC > very early on introduces a spurious conversion from signed char to > int. For instance, given: > > void* f (signed char x) > { > extern void* ff (signed char) __attribute__ ((alloc_size (1))); > > if (-3 <= x && x <= 7) > x = -4; > > return ff (x); > } > > the tree-original dump on x86_64 is as follows (note the (int)x > cast): > > ;; Function f (null) > ;; enabled by -tree-original > { > extern void * ff (signed char); > > if ((unsigned char) x + 3 <= 10) > { > x = -4; > } > return ff ((int) x); <<< spurious cast > } > > while the following the same dump on powerpc64le (no cast): > > ;; Function f (null) > ;; enabled by -tree-original > { > extern void * ff (signed char); > > if ((unsigned char) x + 3 <= 10) > { > x = -4; > } > > return ff (x); <<< no cast > } Those differences could well be related to promotions implied by the ABI in use on each target. Those certainly happen too, they're just not as common a problem as the logical_op_short_circuit stuff. > > The cast makes its way to VRP where it causes the range on x > to be lost in the conversion to the temporary it introduces: > > Found new range for x_4: ~[-3, 7] > marking stmt to be not simulated again > > Visiting statement: > _9 = (int) x_4; > Meeting > [-128, -4] > and > [8, 127] > to > [-128, 127] > Found new range for _9: [-128, 127] <<< anti-range lost Seems like a problem. Given that we're converting to a wider type, we ought to be able to do better and not lose the anti range. > ... > Visiting PHI node: prephitmp_10 = PHI <_9(3), -4(2)> > ... > Meeting > [-128, 127] > and > [-4, -4] > to > [-128, 127] > Intersecting > [-128, 127] > and > [-128, 127] > to > [-128, 127] > Found new range for prephitmp_10: [-128, 127] > ... > Visiting statement: > _8 = ff (prephitmp_10); ISTM that ~[-3,7] U -4 should result in ~[-3,7], right? The union/intersection of ANTI ranges looked like it could use some improvement when I was in there last month, so I wouldn't be surprised if it needs some improvement. > > Debugging tree-vrp.c suggests that this is a deficiency in > the extract_range_from_unary_expr function the computes the > range of the result of the NOP_EXPR (the cast) of an SSA_NAME > with an anti-range ~[A, B] as a union of [TYPE_MIN, A - 1] and > [B + 1, TYPE_MAX], which for ~[-3, 7] and the signed char x in > the test case results in [SCHAR_MIN, -4] U [8, SCHAR_MAX], or > [SCHAR_MIN, SCHAR_MAX]. There's a comment in the function > that reads: > > /* Now canonicalize anti-ranges to ranges when they are not > symbolic and express op ~[] as (op []') U (op []''). */ > > indicating this is deliberate but I have no idea why or if > it's important. Canonicalization of anti-ranges into the union of two normal ranges is deliberate -- and in fact is the representation we want long term. Ideally anti-ranges simply won't exist by the time we're through gcc-8 stage1. But even with canonicalization I don't think we generally do a good job with anti ranges. However, I'm not sure why we're doing that unioning -- I'm not real familiar with this code, but ISTM we just take the > > After adding code to handle this case none of the VRP tests > broke so maybe it's just an oversight. But even with this > case handled and the anti-range propagated to the result of > the cast, there's still a problem. The vrp_visit_phi_node > function that updates the range of the result of a PHI node > merges the existing range of the LHS with the range of the > PHI. This again results in the anti-range being lost and > replaced with that of the type of the RHS (signed char here). > Removing the merging the prephitmp_10 result of the cast has > the same anti-range as the original and the warning in the > test goes away even on x86_64. I have to do more testing to > make sure these changes don't break anything. I am yet to > look into the code that's responsible for the spurious cast > to begin with. That should be removed independent of the > VRP bug(s). If this is in VRP2, then yea, we need to merge the PHI args with the LHS of the PHI's range from the previous iteration. That code is fairly weak. So I wouldn't be surprised if nailing this requires multiple patches and perhaps representational work from Aldy. If we need the latter, then it's definitely gcc-8 material. jeff
diff --git a/gcc/testsuite/gcc.dg/attr-alloc_size-11.c b/gcc/testsuite/gcc.dg/attr-alloc_size-11.c index fac8b18..456ade6 100644 --- a/gcc/testsuite/gcc.dg/attr-alloc_size-11.c +++ b/gcc/testsuite/gcc.dg/attr-alloc_size-11.c @@ -46,9 +46,8 @@ typedef __SIZE_TYPE__ size_t; } typedef void dummy /* Require a semicolon. */ /* The following tests fail because of missing range information. */ -TEST (signed char, SCHAR_MIN + 2, ALLOC_MAX); /* { dg-warning "argument 1 range \\\[13, \[0-9\]+\\\] exceeds maximum object size 12" "missing range info for signed char" { xfail *-*-* } } */ -TEST (short, SHRT_MIN + 2, ALLOC_MAX); /* { dg-warning "argument 1 range \\\[13, \[0-9\]+\\\] exceeds maximum object size 12" "missing range info for short" { xfail *-*-* } } */ - +TEST (signed char, SCHAR_MIN + 2, ALLOC_MAX); /* { dg-warning "argument 1 range \\\[13, \[0-9\]+\\\] exceeds maximum object size 12" "missing range info for signed char" { xfail { ! { aarch64*-*-* ia64-*-* powerpc*-*-* s390x-*-*} } } } */ +TEST (short, SHRT_MIN + 2, ALLOC_MAX); /* { dg-warning "argument 1 range \\\[13, \[0-9\]+\\\] exceeds maximum object size 12" "missing range info for short" { xfail { ! { aarch64*-*-* ia64-*-* powerpc*-*-* s390x-*-*} } } } */ TEST (int, INT_MIN + 2, ALLOC_MAX); /* { dg-warning "argument 1 range \\\[13, \[0-9\]+\\\] exceeds maximum object size 12" } */ TEST (int, -3, ALLOC_MAX); /* { dg-warning "argument 1 range \\\[13, \[0-9\]+\\\] exceeds maximum object size 12" } */ TEST (int, -2, ALLOC_MAX); /* { dg-warning "argument 1 range \\\[13, \[0-9\]+\\\] exceeds maximum object size 12" } */
As stated in the PR, this test now passes on aarch64, ia64, powerpc, and s390x. This patch disables the xfails for those targets. Tested on powerpc64-linux {-m32,-m64}. Is this okay for trunk? Segher 2017-02-09 Segher Boessenkool <segher@kernel.crashing.org> gcc/testsuite/ PR testsuite/79356 * gcc.dg/attr-alloc_size-11.c: Don't xfail on aarch64, ia64, powerpc, or s390x. --- gcc/testsuite/gcc.dg/attr-alloc_size-11.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)