[PATCH 1/5] testsuite: attr-alloc_size-11.c (PR79356)

Jeff Law law@redhat.com
Tue Mar 14 19:39:00 GMT 2017


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



More information about the Gcc-patches mailing list