[PATCH] D141579: [PGO] incorrect classof in InstrProfIncrementInst

[ZhouKui via Phabricator via llvm-commits]

KuiChou added a comment.

In D141579#4063843 <https://reviews.llvm.org/D141579#4063843>, @ellis wrote:

> In D141579#4061009 <https://reviews.llvm.org/D141579#4061009>, @KuiChou wrote:
>
>> In D141579#4058984 <https://reviews.llvm.org/D141579#4058984>, @tejohnson wrote:
>>
>>> Thanks for adding a test, couple questions below, but generally lgtm.
>>>
>>> In D141579#4057747 <https://reviews.llvm.org/D141579#4057747>, @KuiChou wrote:
>>>
>>>> In D141579#4047994 <https://reviews.llvm.org/D141579#4047994>, @ellis wrote:
>>>>
>>>>> Ah, I see the `dyn_cast` you are talking about is here <https://github.com/llvm/llvm-project/blob/9f1bb307da6fec6fc8123bdec79ecec28fced874/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp#L548-L549>. I think this is only a problem if `InstrProfIncrementInstStep` is the only InstrProf instruction in a function **and** it has an `llvm.instrprof.value.profile` instruction, which explains why we didn't catch this bug for so long. In that case, the later call to `getOrCreateRegionCounters()` will be skipped and, according to the comment, this is required for value profiling. Is that what you observed? If so it should be easy to add a small test case, probably similar to `llvm/test/Instrumentation/InstrProfiling/early-exit.ll`, but this could be followed up in another patch. Thanks for fixing!
>>>>
>>>> Yes. But even if `InstrProfIncrementInstStep` is the only InstrProf instruction in a function, `getOrCreateRegionCounters` will still be called through `InstrProfiling::run` -> `lowerIntrinsics` -> `lowerIncrement` -> `getCounterAddress` -> `getOrCreateRegionCounters`. So, to reproduce this problem, we should satisfy two conditions:
>>>>
>>>> 1. `InstrProfIncrementInstStep` is the only InstrProf instruction in a function **and** it has `InstrProfValueProfileInst` instruction
>>>
>>> I assume this should be "is the only InstrProfIncrement instruction" (since there is another InstrProf instruction in the function (InstrProfValueProfileInst)?
>>>
>>>> 2. `InstrProfValueProfileInst` must be **in front of** `InstrProfIncrementInstStep` instruction
>>>>
>>>> PGO will always generate an `InstrProfIncrementInst` instruction in the front of a function so normally, this bug cannot happen
>>>
>>> Out of curiosity, how was this encountered?
>>
>>
>>
>> 1. Yes, you are right. It should be //"InstrProfIncrementInstStep is the only **InstrProfIncrement** instruction in a function and it has InstrProfValueProfileInst instruction"//
>> 2. I have been trying to implement **function reordering** in LLVM PGO which will modify current PGO code, this cause a bug in my code.
>
> That's funny because I am also working on extending LLVM PGO to instrument function timestamps which will be used to reorder functions for optimization. I assume this is similar to what you are working on?

Oh that's great! What's I'm working on is to reorder functions through function call graph and call times. It's still in progress.

> I'm hoping to send up an RFC to https://discourse.llvm.org/ soon so please be on the lookout for that.

OK. I'll keep an eye on it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141579