[PATCH] D115847: [mlgo][inline] Improve global state tracking

Mon Jan 10 11:37:13 PST 2022

aeubanks added a comment.

In D115847#3232394 <https://reviews.llvm.org/D115847#3232394>, @mtrofin wrote:

> I think there are 2 questions, is that correct? If so:
>
> In D115847#3228512 <https://reviews.llvm.org/D115847#3228512>, @aeubanks wrote:
>
>>> This patch avoids relying on analysis invalidation because it turned out to be too aggressive in some cases.
>>
>> Can you explain this a bit more?
>
> This refers to the behavior before. Invalidation of the whole analysis (the MLInlineAdvisor one) was still happening too frequently.
>
>> We shouldn't need to completely recalculate everything when adding a new function. Any new functions have to be children of the current SCC, so we should be able to find them by iterating through the outgoing edges from the current SCC and seeing if we encounter any nodes we haven't seen before.
>
> I think you're referring here to the behavior in the patch, correct (i.e. not to the quoted sentence in the patch description)?
>
> I would prefer not assuming that SCC passes adding functions relate them to the SCC they are currently processing. I realize that's currently the case, but this assumption made me uncomfortable:  I am not sure why this is something that a pass would necessarily respect in the future.
>
> I think there is a partial update approach that is only dependent on the fact that Nodes are only added for the duration of a module-wide cgscc traversal: we remember a "watermark" of the last node and iterate from that watermark up next time we re-enter the inliner. That's a change that can be done incrementally though, from the current change.
>
> In D115847#3232350 <https://reviews.llvm.org/D115847#3232350>, @aeubanks wrote:
>
>> In D115847#3228603 <https://reviews.llvm.org/D115847#3228603>, @mtrofin wrote:
>>
>>> I think there are 2 questions, is that correct? If so:
>>>
>>> In D115847#3228512 <https://reviews.llvm.org/D115847#3228512>, @aeubanks wrote:
>>>
>>>>> This patch avoids relying on analysis invalidation because it turned out to be too aggressive in some cases.
>>>>
>>>> Can you explain this a bit more?
>>>
>>> This refers to the behavior before. Invalidation of the whole analysis (the MLInlineAdvisor one) was still happening too frequently.
>>
>> Oh I thought this was in reference to `FunctionPropertiesAnalysis` being invalidated too much.
>>
>>>> We shouldn't need to completely recalculate everything when adding a new function. Any new functions have to be children of the current SCC, so we should be able to find them by iterating through the outgoing edges from the current SCC and seeing if we encounter any nodes we haven't seen before.
>>>
>>> I think you're referring here to the behavior in the patch, correct (i.e. not to the quoted sentence in the patch description)?
>>>
>>> I would prefer not assuming that SCC passes adding functions relate them to the SCC they are currently processing. I realize that's currently the case, but this assumption made me uncomfortable:  I am not sure why this is something that a pass would necessarily respect in the future.
>>
>> An SCC pass should not be adding functions that don't relate to some function in the current SCC. That wouldn't fit the CGSCC pass model and would break LCG. So I think looking around all the nodes in the current SCC to discover new functions is fine.
>
> Ah, ok, so should such a pass try to get introduced, some test(s) would fail, I assume. OK. Let me try and make sure I understand how this check would work:
>
> - I keep track of the set of Nodes I've seen last, and the total set of Nodes
> - when onPassEntry is called again, I check that the set of Nodes adjacent (through what? Ref or Call edges? I guess both) to the Nodes in the set above are in the total set of Nodes, otherwise I found new nodes

yeah it could be a ref or a call edge, and theoretically we could add functions that are only referenced from another added function (e.g. A becomes A'->A1->A2), although LCG doesn't currently support that

> ... (kind of obvious what follows)
>
> Is this correct? (thanks!)

yup sounds right to me

>>> I think there is a partial update approach that is only dependent on the fact that Nodes are only added for the duration of a module-wide cgscc traversal: we remember a "watermark" of the last node and iterate from that watermark up next time we re-enter the inliner. That's a change that can be done incrementally though, from the current change.
>>
>> later changes sgtm.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115847/new/

https://reviews.llvm.org/D115847