[PATCH] D21462: [PM] Make the the new pass manageg support fully generic extra arguments to run methods, both for transform passes and analysis passes.

Thu Aug 4 03:37:54 PDT 2016

On Thu, Aug 4, 2016 at 3:03 AM, Chandler Carruth <chandlerc at gmail.com>
wrote:

>
>
> On Thu, Aug 4, 2016 at 1:33 AM Sean Silva via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> On Thu, Aug 4, 2016 at 12:45 AM, Chandler Carruth <chandlerc at gmail.com>
>> wrote:
>>
>>> On Thu, Aug 4, 2016 at 12:32 AM Sean Silva <chisophugis at gmail.com>
>>> wrote:
>>>
>>>> silvas added a comment.
>>>>
>>>> In https://reviews.llvm.org/D21462#505454, @chandlerc wrote:
>>>>
>>>> > > I hear your argument (IIRC we discussed this at a social) about
>>>> back pointers being somewhat annoying, but the ability to use just a
>>>> `void*` to represent the information passed to an analysis makes it much
>>>> easier to have a unified analysis manager.
>>>> >
>>>> >
>>>> > I don't think I understand the connection you see between up pointers
>>>> and using a 'void*' to represent the information passed to an analysis. Can
>>>> you re-explain this? Sorry if it just isn't clicking for me....
>>>> >
>>>> > That said, in general I would like to not sacrifice type safety
>>>> because of concerns around mapping arguments from one interface to another
>>>> though.
>>>>
>>>>
>>>> The issue is that for a unified analysis manager, AnalysisPassConcept
>>>> is not templated (on IRUnitT and couldn't be on the ExtraArgTs). Therefore,
>>>> we need to pass a type-erased object through its `run` method. Currently,
>>>> the type-erased IRUnit can be passed as a void*, and then the
>>>> AnalysisPassModel (which *is* templated on IRUnitT) just casts it back
>>>> since it knows the right type.
>>>> At least with the current registration stuff, all we need right now for
>>>> type-safety is tracking which IRUnit it is (and there are 4 discrete values
>>>> which is easy to track).
>>>>
>>>> Or to put it another way, the ideas you are running with in this patch
>>>> is very intimately tied to having the analysis manager templated on the
>>>> signature of the associated `run` method for analyses it manages. A unified
>>>> analysis manager (which seems like it was the agreed upon design for
>>>> solving dependency tracking)
>>>
>>>
>>> There was no agreement to this. I specifically said I don't at this
>>> point agree with that approach, and that I think it is too soon to try to
>>> design that approach.
>>>
>>
>> Then you need to bring this up on llvmdev for discussion. We had a thread
>> about this, and I have clearly been moving forward with this approach after
>> there seemed to be some agreement on it. If you disagree you need to bring
>> that up.
>>
>
> I think I already did in my response to your thread here:
> http://lists.llvm.org/pipermail/llvm-dev/2016-July/102245.html
> <http://www.google.com/url?q=http%3A%2F%2Flists.llvm.org%2Fpipermail%2Fllvm-dev%2F2016-July%2F102245.html&sa=D&sntz=1&usg=AFQjCNFALEelNf-foMSoSYHh_roMivDlPA>
>
> The subsequent discussion
>

The subsequent discussion also contained Hal saying:
"I'll go back to my previous position: we need a general dependency graph
built as the analysis cache is used"
http://lists.llvm.org/pipermail/llvm-dev/2016-July/102333.html

With Mehdi replying
"I share the same feeling."
http://lists.llvm.org/pipermail/llvm-dev/2016-July/102334.html

And no objection from you.

Your only post later on this topic was:
http://lists.llvm.org/pipermail/llvm-dev/2016-July/102339.html
which was largely about terminology, and some hand-waving of solutions
which I explained were inadequate:
http://lists.llvm.org/pipermail/llvm-dev/2016-July/102346.html

And again, you did not provide any response.

> seemed to center around LCSSA and LoopSimplify which pose separate
> problems, or the retention of Loop object pointers which also pose
> different problems.
>

Retention of Loop object pointers is exactly the same problem. If an
analysis result A has a pointer to any object whose lifetime ends when
another analysis result B is invalidated, then A must be invalidated when B
is invalidated. There is no way around this.

Just because we could hypothetically refactor a large part of the codebase
to avoid some specific instances of the issue doesn't mean that we don't
need to solve it. And if we need to solve it anyway, then the argument for
doing such a refactoring is quite weak, since it isn't needed.

> I continue to think the plan at the bottom of that message is the correct
> way forward until we do the mentioned refactorings of analyses and some of
> the infrastructure stabilizes.
>

There are a few cases for which it is not sufficient to just pass in
through the query path.
Also, it's not clear if this is feasible e.g. for AA where different AA's
have different information needed by their query paths.

Also, are you suggesting that we need to modify every query of each of the
following analyses before the new PM can be used with basic confidence that
there aren't going to be use-after-free errors?

BFI
SCEV
BasicAA
LVI
DemandedBits
MemDep
MemorySSA
SCEVAA
LoopAccessAnalysis
DependenceAnalysis

-- Sean Silva

>
>
>>
>> -- Sean Silva
>>
>>
>>>
>>>
>>>> must type-erase the information you have baked into the template
>>>> arguments of AnalysisPassConcept here.
>>>> At least with the way that the registration stuff works now, you would
>>>> need to find a way to type-erase the exact signature of the `run` method in
>>>> order to get type-safety.
>>>>
>>>
>>> Much of this is why I don't agree with the design.
>>>
>>> I'm happy to actually work on a solution to the problem, but right now
>>> I'm in large part waiting on review of these patches.
>>>
>>> If in fact we need to go back and type erase things we can always create
>>> a new patch to move in that direction.
>>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160804/7f98b411/attachment.html>