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 <seg...@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 <seg...@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
