[PGO] incorrect classof in InstrProfIncrementInst

[KuiChou]

Summary

Class InstrProfIncrementInstStep inherits from InstrProfIncrementInst but cannot cast to InstrProfIncrementInst, because InstrProfIncrementInst::classof does not cover such circumstance。

Function InstrProfiling::run traverse all instruction in a module and try to cast them to InstrProfIncrementInst using dyn_cast, but it will return nullptr if the instruction is InstrProfIncrementInstStep(subclass of InstrProfIncrementInst).

[tejohnson]

Can you upload with full context? Either use Arcanist or follow instructions here: https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Thanks!

[ellis]

Ah, I see the dyn_cast you are talking about is here. 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!

[tejohnson]

In D141579#4047994, @ellis wrote:

Ah, I see the dyn_cast you are talking about is here. 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!

Why not include the suggested test in this patch?

[ellis]

In D141579#4048018, @tejohnson wrote:

In D141579#4047994, @ellis wrote:

Ah, I see the dyn_cast you are talking about is here. 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!

Why not include the suggested test in this patch?

You're right. I suppose fewer patches is better and I think it should be an easy test to add.

[KuiChou]

In D141579#4047994, @ellis wrote:

Ah, I see the dyn_cast you are talking about is here. 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
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

[KuiChou]

add test case

[tejohnson]

Thanks for adding a test, couple questions below, but generally lgtm.

In D141579#4057747, @KuiChou wrote:

In D141579#4047994, @ellis wrote:

Ah, I see the dyn_cast you are talking about is here. 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?

llvm/test/Instrumentation/InstrProfiling/before-value-profile-lowering.ll

1 ↗

(On Diff #489709)

s/genereated/generated/

Also, suggest expanding the comment to clarify the exact scenario being tested, e.g. something like: ", when the only instrprof increment is a step instruction following a value profile instruction."

[KuiChou]

In D141579#4058984, @tejohnson wrote:

Thanks for adding a test, couple questions below, but generally lgtm.

In D141579#4057747, @KuiChou wrote:

In D141579#4047994, @ellis wrote:

Ah, I see the dyn_cast you are talking about is here. 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.

[KuiChou]

update test case

[tejohnson]

lgtm

[ellis]

In D141579#4061009, @KuiChou wrote:

In D141579#4058984, @tejohnson wrote:

Thanks for adding a test, couple questions below, but generally lgtm.

In D141579#4057747, @KuiChou wrote:

In D141579#4047994, @ellis wrote:

Ah, I see the dyn_cast you are talking about is here. 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.

[KuiChou]

This comment was removed by KuiChou.

[KuiChou]

In D141579#4063843, @ellis wrote:

In D141579#4061009, @KuiChou wrote:

In D141579#4058984, @tejohnson wrote:

Thanks for adding a test, couple questions below, but generally lgtm.

In D141579#4057747, @KuiChou wrote:

In D141579#4047994, @ellis wrote:

Ah, I see the dyn_cast you are talking about is here. 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.

[KuiChou]

fix test error

[KuiChou]

Thank you for the reviews! Could one of you commit this revision for me as I do not have commit access?

[ellis]

In D141579#4065615, @KuiChou wrote:

Thank you for the reviews! Could one of you commit this revision for me as I do not have commit access?

Sure I can commit this today.

llvm/include/llvm/IR/IntrinsicInst.h

 class InstrProfIncrementInst : public InstrProfInstBase {
 public:
   static bool classof(const IntrinsicInst *I) {
-    return I->getIntrinsicID() == Intrinsic::instrprof_increment;
+    return I->getIntrinsicID() == Intrinsic::instrprof_increment ||
+           I->getIntrinsicID() == Intrinsic::instrprof_increment_step;
   }
   static bool classof(const Value *V) {
     return isa<IntrinsicInst>(V) && classof(cast<IntrinsicInst>(V));