[OpenMP][InstrProfiling] Fix a missing instr profiling counter

[lxfind]

Summary

When emitting a function body there needs to be a instr profiling counter emitted. Otherwise instr profiling won't work for this function.

[MaskRay]

I know nearly nothing about OpenMP..

You need a %clang_cc1 -disable-llvm-passes test to demonstrate the codegen change (call void @llvm.instrprof.increment({{.*}})). Oh, it is unfortunate that there isn't sufficient llvm.instrprof.increment test under clang/test

[lxfind]

add test

[MaskRay]

LG, but I hope an OpenMP expert can take a look. Perhaps @vsk can comment on where the tests should be improved (currently we hardly have any @llvm.instrprof.increment IR test for clang -fprofile-instr-generate).

clang/test/OpenMP/omp_with_loop_pragma_instr_profile.c

17

Nit: append a trailing ( so that it's clear the instruction is llvm.instrprof.increment, not llvm.instrprof.increment.step.

[lxfind]

address comment

[lxfind]

Thanks. I am landing it.
But feel free to comment here if anything isn't right. @ABataev

[ABataev]

There is a problem. We actually do not emit S here directly, instead, we use CodeGen lambdas, which may not be equal to S, in some cases S is nullptr here. It may result in not quite accurate results.

[lxfind]

In D98135#2617432, @ABataev wrote:

There is a problem. We actually do not emit S here directly, instead, we use CodeGen lambdas, which may not be equal to S, in some cases S is nullptr here. It may result in not quite accurate results.

Thanks for the note!
I don't really know anything about OMP though, not sure how to handle it.
Would you mind taking a look at this issue? Feel free to send a different patch!

[ABataev]

In D98135#2617446, @lxfind wrote:

In D98135#2617432, @ABataev wrote:

There is a problem. We actually do not emit S here directly, instead, we use CodeGen lambdas, which may not be equal to S, in some cases S is nullptr here. It may result in not quite accurate results.

Thanks for the note!
I don't really know anything about OMP though, not sure how to handle it.
Would you mind taking a look at this issue? Feel free to send a different patch!

Could you create a PR for the problem so we could track it?

[lxfind]

In D98135#2617448, @ABataev wrote:

In D98135#2617446, @lxfind wrote:

In D98135#2617432, @ABataev wrote:

There is a problem. We actually do not emit S here directly, instead, we use CodeGen lambdas, which may not be equal to S, in some cases S is nullptr here. It may result in not quite accurate results.

Thanks for the note!
I don't really know anything about OMP though, not sure how to handle it.
Would you mind taking a look at this issue? Feel free to send a different patch!

Could you create a PR for the problem so we could track it?

https://bugs.llvm.org/show_bug.cgi?id=49521

[ABataev]

Thanks!

[vsk]

In D98135#2615492, @MaskRay wrote:

LG, but I hope an OpenMP expert can take a look. Perhaps @vsk can comment on where the tests should be improved (currently we hardly have any @llvm.instrprof.increment IR test for clang -fprofile-instr-generate).

This doesn't necessarily indicate a gap in test coverage. There are plenty of tests under clang/test/Profile that cover IR generation for profiling instrumentation.

[lxfind]

@ABataev, wondering if you have a timeline on this?
Missing counters from OMP functions sometimes cause llvm-cov to crash because of data inconsistency.

[ABataev]

In D98135#2650914, @lxfind wrote:

@ABataev, wondering if you have a timeline on this?
Missing counters from OMP functions sometimes cause llvm-cov to crash because of data inconsistency.

Cannot answer right now. It would be much easier to fix this if you could provide additional info about what tests are exactly failed, what are the constructs that do not support it, etc.

[lxfind]

In D98135#2650940, @ABataev wrote:

In D98135#2650914, @lxfind wrote:

@ABataev, wondering if you have a timeline on this?
Missing counters from OMP functions sometimes cause llvm-cov to crash because of data inconsistency.

Cannot answer right now. It would be much easier to fix this if you could provide additional info about what tests are exactly failed, what are the constructs that do not support it, etc.

Yes the whole pipeline is a bit long and complex, so I don't have an exact repro in hand because it requires source code and run it.

But let me try to explain what happened in my observation. There are two sections that are related to this issue in the binary, the IPSK_covfun section that contains the function records, and the IPSK_name section that contains the list of all function names. The issue here is that some OMP functions that are found in the IPSK_covfun section are not found in the IPSK_name section.

The records in IPSK_covfun are generated like this:

Whenever CodeGenFunction is generating code for any function, it will first call the CodeGenFunction::GenerateCode() function, in which it will call PGO.assignRegionCounters(GD, CurFn);: https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/clang/lib/CodeGen/CodeGenFunction.cpp#L1329

From there, it will call emitCounterRegionMapping:
https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/clang/lib/CodeGen/CodeGenPGO.cpp#L819

which will then call: CGM.getCoverageMapping()->addFunctionMappingRecord:
https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/clang/lib/CodeGen/CodeGenPGO.cpp#L890

which will eventually add this function to a FunctionRecords:
https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/clang/lib/CodeGen/CoverageMappingGen.cpp#L1655

So all above is to show that every function will eventually be added to FunctionRecords, unless in the case where a function is explicitly marked as unused. The FunctionRecords will eventually be all written into the IPSK_covfun section in the binary.

The names in IPSK_name section are generated like this:
Within InstrProfiling.cpp (https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp), it collects all the function names referenced in all instrumentation counter increments instructions:
https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp#L920
Basically for every InstrProfIncrementInst, it adds the function name referenced in it to a list ReferencedNames.
Then in the end this ReferencedNames is written into the IPSK_name section.

Now you can see that for the OMP functions that don't have any counters, they are still added to FunctionRecords, but not added to ReferencedNames, because they are not referenced by any InstrProfIncrementInst.
During the running of llvm-cov, when reading the list of function records, it will attempt to look up the name of the function from the function name list:
https://github.com/llvm/llvm-project/blob/b9ff67099ad6da931976e66f1510c5af2558a86e/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp#L560

And it will not be able to find it for the OMP case, so it will return an error.

Overall this is very complex and a bit fragile to me. For instance, we probably could have detected the error much earlier during Instrumentation pass in LLVM, that some function records' names are not in the name list. Or we could simply construct the list of function names based on the function records. But currently these two are generated independently.
cc @MaskRay and @vsk, maybe they have thoughts on this.

[ABataev]

In D98135#2651193, @lxfind wrote:

In D98135#2650940, @ABataev wrote:

In D98135#2650914, @lxfind wrote:

@ABataev, wondering if you have a timeline on this?
Missing counters from OMP functions sometimes cause llvm-cov to crash because of data inconsistency.

Cannot answer right now. It would be much easier to fix this if you could provide additional info about what tests are exactly failed, what are the constructs that do not support it, etc.

Yes the whole pipeline is a bit long and complex, so I don't have an exact repro in hand because it requires source code and run it.

But let me try to explain what happened in my observation. There are two sections that are related to this issue in the binary, the IPSK_covfun section that contains the function records, and the IPSK_name section that contains the list of all function names. The issue here is that some OMP functions that are found in the IPSK_covfun section are not found in the IPSK_name section.

The records in IPSK_covfun are generated like this:

Whenever CodeGenFunction is generating code for any function, it will first call the CodeGenFunction::GenerateCode() function, in which it will call PGO.assignRegionCounters(GD, CurFn);: https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/clang/lib/CodeGen/CodeGenFunction.cpp#L1329

From there, it will call emitCounterRegionMapping:
https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/clang/lib/CodeGen/CodeGenPGO.cpp#L819

which will then call: CGM.getCoverageMapping()->addFunctionMappingRecord:
https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/clang/lib/CodeGen/CodeGenPGO.cpp#L890

which will eventually add this function to a FunctionRecords:
https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/clang/lib/CodeGen/CoverageMappingGen.cpp#L1655

So all above is to show that every function will eventually be added to FunctionRecords, unless in the case where a function is explicitly marked as unused. The FunctionRecords will eventually be all written into the IPSK_covfun section in the binary.

The names in IPSK_name section are generated like this:
Within InstrProfiling.cpp (https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp), it collects all the function names referenced in all instrumentation counter increments instructions:
https://github.com/llvm/llvm-project/blob/e5f51fdd650c6d20c81fedb8e856e9858aa10991/llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp#L920
Basically for every InstrProfIncrementInst, it adds the function name referenced in it to a list ReferencedNames.
Then in the end this ReferencedNames is written into the IPSK_name section.

Now you can see that for the OMP functions that don't have any counters, they are still added to FunctionRecords, but not added to ReferencedNames, because they are not referenced by any InstrProfIncrementInst.
During the running of llvm-cov, when reading the list of function records, it will attempt to look up the name of the function from the function name list:
https://github.com/llvm/llvm-project/blob/b9ff67099ad6da931976e66f1510c5af2558a86e/llvm/lib/ProfileData/Coverage/CoverageMappingReader.cpp#L560

And it will not be able to find it for the OMP case, so it will return an error.

Overall this is very complex and a bit fragile to me. For instance, we probably could have detected the error much earlier during Instrumentation pass in LLVM, that some function records' names are not in the name list. Or we could simply construct the list of function names based on the function records. But currently these two are generated independently.
cc @MaskRay and @vsk, maybe they have thoughts on this.

Ok, now I see. I think you can restore this patch, it should be enough to fix the problem. Just check that S is not nullptr before calling CGF.incrementProfileCounter(S);.

[lxfind]

check null on S

[ABataev]

LG

clang/lib/CodeGen/CGOpenMPRuntime.cpp

 }
 
 LValue CGOpenMPRegionInfo::getThreadIDVariableLValue(CodeGenFunction &CGF) {
   return CGF.EmitLoadOfPointerLValue(
       CGF.GetAddrOfLocalVar(getThreadIDVariable()),
       getThreadIDVariable()->getType()->castAs<PointerType>());
 }
 
-void CGOpenMPRegionInfo::EmitBody(CodeGenFunction &CGF, const Stmt * /*S*/) {
+void CGOpenMPRegionInfo::EmitBody(CodeGenFunction &CGF, const Stmt *S) {
   if (!CGF.HaveInsertPoint())
     return;
   // 1.2.2 OpenMP Language Terminology
   // Structured block - An executable statement with a single entry at the
   // top and a single exit at the bottom.
   // The point of exit cannot be a branch out of the structured block.
   // longjmp() and throw() must not violate the entry/exit criteria.
   CGF.EHStack.pushTerminate();
+  if (S)
+    CGF.incrementProfileCounter(S);
   CodeGen(CGF);
   CGF.EHStack.popTerminate();
 }
 
 LValue CGOpenMPTaskOutlinedRegionInfo::getThreadIDVariableLValue(
     CodeGenFunction &CGF) {
   return CGF.MakeAddrLValue(CGF.GetAddrOfLocalVar(getThreadIDVariable()),
                             getThreadIDVariable()->getType(),

clang/test/OpenMP/omp_with_loop_pragma_instr_profile.c

+// RUN: %clang_cc1 -verify -fopenmp -x c -emit-llvm %s -triple x86_64-unknown-linux -o - -femit-all-decls -disable-llvm-passes -fprofile-instrument=clang | FileCheck %s
+// expected-no-diagnostics
+
+void sub(double *restrict a, double *restrict b, int n) {
+  int i;
+
+#pragma omp parallel for
+#pragma clang loop vectorize(disable)
+  for (i = 0; i < n; i++) {
+    a[i] = a[i] + b[i];
+  }
+}
+
+// CHECK-LABEL: @.omp_outlined.(
+// CHECK-NEXT:  entry:
+// CHECK:         call void @llvm.instrprof.increment(
+// CHECK:       omp.precond.then:
+// CHECK-NEXT:    call void @llvm.instrprof.increment(
+// CHECK:       cond.true:
+// CEHCK-NEXT:    call void @llvm.instrprof.increment(
+// CHECK:       omp.inner.for.body:
+// CHECK-NEXT:    call void @llvm.instrprof.increment(