[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? I'm hoping to send up an RFC to https://discourse.llvm.org/ soon so please be on the lookout for that.

Oh that's great! I'am looking forward to your work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141579