replacing the backwards threader and more

[Martin Sebor]

On 6/28/21 12:17 AM, Aldy Hernandez wrote:
> 
> 
> On 6/25/21 7:19 PM, Martin Sebor wrote:
>> On 6/25/21 10:20 AM, Aldy Hernandez via Gcc wrote:
>>> Hi folks.
>>>
>>> I'm done with benchmarking, testing and cleanups, so I'd like to post 
>>> my patchset for review.  However, before doing so, I'd like to 
>>> address a handful of meta-issues that may affect how I post these 
>>> patches.
>>>
>>> Trapping on differences
>>> =======================
>>>
>>> Originally I wanted to contribute verification code that would trap 
>>> if the legacy code threaded any edges the new code couldn't (to be 
>>> removed after a week).  However, after having tested on various 
>>> architectures and only running once into a missing thread, I'm 
>>> leaning towards omitting the verification code, since it's fragile, 
>>> time consuming, and quite hacky.
>>>
>>> For the record, I have tested on x86-64, aarch64, ppc64 and ppc64le. 
>>> There is only one case, across bootstrap and regression tests where 
>>> the verification code is ever tripped (discussed below).
>>>
>>> Performance
>>> ===========
>>>
>>> I re-ran benchmarks as per our callgrind suite, and the penalty with 
>>> the current pipeline is 1.55% of overall compilation time.  As is 
>>> being discussed, we should be able to mitigate this significantly by 
>>> removing other threading passes.
>>>
>>> Failing testcases
>>> =================
>>>
>>> I have yet to run into incorrect code being generated, but I have had 
>>> to tweak a considerable number of tests.  I have verified every 
>>> single discrepancy and documented my changes in the testsuite when it 
>>> merited doing so.  However, there are a couple tests that trigger 
>>> regressions and I'd like to ask for guidance on how to address them.
>>>
>>> 1. gcc.c-torture/compile/pr83510.c
>>>
>>> I would like to XFAIL this.
>>>
>>> What happens here is that thread1 threads a switch statement such 
>>> that the various cases have been split into different independent 
>>> blocks. One of these blocks exposes an arr[i_27] access which is 
>>> later propagated by VRP to be arr[10].  This is an invalid access, 
>>> but the array bounds code doesn't know it is an unreachable path.
>>
>> The test has a bunch of loops that iterate over the 10 array elements.
>> There have been bug reports about loop unrolling causing false positives
>> -Warray-bounds (e.g., PR 92539, 92110, or 86341) so this could be
>> the same issue.
>>
>>>
>>> However, it is not until dom2 that we "know" that the value of the 
>>> switch index is such that the path to arr[10] is unreachable.  For 
>>> that matter, it is not until dom3 that we remove the unreachable path.
>>
>> If you do XFAIL it can you please isolate a small test case and open
>> a bug and make it a -Warray-bounds blocker?
> 
> Will do.
> 
>>
>>>
>>> 2. -Wfree-nonheap-object
>>>
>>> This warning is triggered while cleaning up an auto_vec.  I see that 
>>> the va_heap::release() inline is wrapped with a pragma ignore 
>>> "-Wfree-nonheap-object", but this is not sufficient because jump 
>>> threading may alter uses in such a way that may_emit_free_warning() 
>>> will warn on the *inlined* location, thus bypassing the pragma.
>>>
>>> I worked around this with a mere:
>>>
>>>  > @@ -13839,6 +13839,7 @@ maybe_emit_free_warning (tree exp)
>>>>    location_t loc = tree_inlined_location (exp);
>>>> +  loc = EXPR_LOCATION (exp);
>>>
>>> but this causes a ton of Wfree-nonheap* tests to fail.  I think 
>>> someone more knowledgeable should address this (msebor??).
>>
>> This sounds like the same problem as PR 98871.  Does the patch below
>> fix it?
>> https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572515.html
>> If so, I suggest getting that patch in first to avoid testsuite
>> failures.  If it doesn't fix it I'll look into it before you commit
>> your changes.
> 
> The latest patch in the above thread does not apply.  Do you have a more 
> recent patch?

The first patch in the series applies cleanly for me:
   https://gcc.gnu.org/pipermail/gcc-patches/2021-June/572516.html
but patch 2/4 does need a few adjustments.  With those and your
latest changes applied I don't see any -Wfree-nonheap-object test
failures.

Martin

> 
>>> 3. uninit-pred-9_b.c
>>>
>>> The uninit code is getting confused with the threading and the bogus 
>>> warning in line 24 is back.  I looked at the thread, and it is correct.
>>>
>>> I'm afraid all these warnings are quite fragile in the presence of 
>>> more aggressive optimizations, and I suspect it will only get worse.
>>
>>  From my recent review of open -Wmaybe-uninitialized bugs (and
>> the code) it does seem to be both fragile and getting worse.  I've
>> only found a few simple problems so far in the code but nothing that
>> would make a dramatic difference so I can't say if it's possible to
>> do much better, but I'm not done or ready to give up.  If you XFAIL
>> this too please open a bug for it and make it a blocker for
>> -Wuninitialized?
> 
> Will do.
> 
> Good to know these are somewhat known issues.
> 
> Thanks.
> Aldy
>