Re: [PATCH 0/2] Ranger-based backwards threader implementation.

[Aldy Hernandez <aldyh@redhat.com>]

On 6/29/21 11:22 PM, Martin Sebor wrote:
> On 6/29/21 4:27 AM, Aldy Hernandez wrote:
>>
>>
>> On 6/29/21 1:19 AM, Martin Sebor wrote:
>>> On 6/28/21 10:21 AM, Aldy Hernandez via Gcc-patches wrote:
>>>> This is the ranger-based backwards threader.  It is divided into two
>>>> parts: the solver and the path discovery bits.
>>>>
>>>> The solver is generic enough, that it may be of use to other passes,
>>>> so it's been abstracted into its own separate class/file.  Andrew and
>>>> I have already gone over it, so I don't think a review is necessary.
>>>> Besides, it's technically an extension of the ranger infrastructure.
>>>>
>>>> On the other hand, the path discovery bits could benefit from the
>>>> watchful eye of the jump threading experts.
>>>>
>>>> Documenting the solver in a [ranger-tech] post is on my TODO list,
>>>> as I think it would be useful as an example of GORI as a general
>>>> tool, outside the VRP world.
>>>>
>>>> As I have mentioned elsewhere, I have gone through each test and
>>>> documented the reasons why they were adjusted (when useful).  The
>>>> reviewer(s) may benefit from looking at the test notes.
>>>>
>>>> I have added a --param=threader-mode={ranger,legacy} option, which I
>>>> hope to remove shortly after.  It has been useful for diagnosing
>>>> issues in the past, though perhaps not so much now.  I've left it
>>>> in case there's a remote interest in using it during stage1, but
>>>> removing it could be a huge cleanup to tree-ssa-threadbackward.c.
>>>>
>>>> If/when accepted, I will open 2-3 PRs with the XFAILed tests as
>>>> requested.  I am still working on distilling a C counterpart for
>>>> the libphobos missing thread edge.  It'll hopefully be ready by the
>>>> time the review is done.
>>>>
>>>> A version of this patchset with the verification code has
>>>> been tested on x86-64, ppc64, ppc64le, and aarch64 (all Linux).
>>>>
>>>> I am currently re-testing on x86-64 Linux, but will not re-test on the
>>>> rest of the architectures because...OMG aarch6 is so slow!
>>>
>>> I applied the series and ran a subset of tests and didn't see any
>>> failures, just the three XPASSes below.  The Wfree-nonheap-object
>>> tests you mentioned in the other post all pass.  Looks like you
>>> got past that problem?
>>>
>>> XPASS: gcc.dg/uninit-pr61112.c pr61112 (test for bogus messages, line 
>>> 32)
>>> XPASS: gcc.dg/uninit-pr61112.c pr61112 (test for bogus messages, line 
>>> 46)
>>> XPASS: gcc.dg/uninit-pr61112.c pr61112 (test for bogus messages, line 
>>> 60)
>>>
>>> A couple of comments on the tests below (I haven't looked at the meat
>>> of the patch):
>>>
>>>>
>>>> Thanks.
>>>> Aldy
>>>>
>>>> Aldy Hernandez (2):
>>>>    Implement basic block path solver.
>>>>    Backwards jump threader rewrite with ranger.
>>>>
>>>>   gcc/Makefile.in                               |   6 +
>>>>   gcc/flag-types.h                              |   7 +
>>>>   gcc/params.opt                                |  17 +
>>>>   .../g++.dg/debug/dwarf2/deallocator.C         |   3 +-
>>>>   gcc/testsuite/gcc.c-torture/compile/pr83510.c |  33 ++
>>>>   gcc/testsuite/gcc.dg/Wrestrict-22.c           |   3 +
>>>
>>> The change here just adds the comment:
>>>
>>> +/* This looks like the threader caused the entire loop to collapse, 
>>> and the
>>> +   warning pass can't determine the arguments to memcpy.  */
>>> +
>>>
>>> Since the test passes I'm not sure I understand what the comment
>>> is trying to say.  Is it still accurate and necessary?
>>
>> This seems like it came from the ranger branch which had slightly 
>> different code, particularly it made use of a full ranger with 
>> equivalences.  It looks like this could have failed in the branch, but 
>> no longer does.  I have removed the comment.
> 
> Okay, thanks.
> 
>>
>>>
>>>>   gcc/testsuite/gcc.dg/loop-unswitch-2.c        |   2 +-
>>>>   gcc/testsuite/gcc.dg/old-style-asm-1.c        |   5 +-
>>>>   gcc/testsuite/gcc.dg/pr68317.c                |   4 +-
>>>>   gcc/testsuite/gcc.dg/pr97567-2.c              |   2 +-
>>>>   gcc/testsuite/gcc.dg/predict-9.c              |   4 +-
>>>>   gcc/testsuite/gcc.dg/shrink-wrap-loop.c       |  53 ++
>>>>   gcc/testsuite/gcc.dg/sibcall-1.c              |  10 +
>>>>   .../gcc.dg/tree-ssa/builtin-sprintf-3.c       |   5 +-
>>>
>>> I wonder if breaking up the test function into five, one for each
>>> of the tests it does, would be a better way to avoid the IL changes
>>> than disabling all the threading passes.  Like in the attached patch.
>>
>> As the author of the original test, I completely differ to you :).
>>
>> Attached is the latest version with your suggested changes, as well as 
>> a gimple FE test for the previously discussed failing libphobos test.
> 
> The tests look good.
> 
> In the new APIs, instead of taking vec by value can you please change
> them to either by-const-reference if they don't change the vec or by-
> reference if they do?  I'm in the midst of changing code to do that
> with the goal of eventually removing all by-value vec arguments.

Sure.

Aldy