Woohoo!

John Mellor-Crummey Professor
Dept of Computer Science Rice University
email: johnmc@rice.edu phone: 713-348-5179

i don’t understand why you find this set of changes preferable to decorating some variable declarations with attribute((unused))

Precisely to that point I was hoping to provide a few compelling counterexamples to demonstrate why potentially wrong information is actually worse than no information.

But I guess what this really boils down to is that all debug information in LLVM IR is (at the moment) "must" information that is supposed to be either 100% reliable or omitted. It sounds like for the kinds of analysis that you are doing, you would also benefit from a second category of "may" information that may or may not be valid. That's a legitimate ask, but if we wanted to include this in LLVM IR, we would need to qualify it as not reliable, so it doesn't, for example, leak into debug info that software developers rely on.

A good example for why arbitrarily picking one location during merging is when the two locations are coming from different inlined instances of different functions (or perhaps even worse: two inlined instances of the same function). I would assume that even in profiling a wrong backtrace would invalidate or render untrustworthy large parts of any analysis being done one this data.

In D60831#1472310, @gbedwell wrote:

In D60831#1472303, @aprantl wrote:

In D60831#1472296, @jmellorcrummey wrote:

As the lead of a project building profiling tools, I am strongly against having any instructions map to line 0.

This is probably not what you meant, but for completeness I feel like I should point out that there are many legitimate situations where LLVM generates a line 0 location. The most prominent example is instruction merging: Since both LLVM IR and DWARF currently require each PC address to map to exactly one source location, LLVM's will insert a line 0 location when it merges two instructions with distinct source locations. I can't speak for profiling, but at least on the debugger side, the consensus is that potentially misleading information is worse than no information, because if there is no way to distinguish "always correct" from "maybe correct" information, the user can't trust any information.

In D60831#1470677, @hfinkel wrote:

In D60831#1470585, @Orlando wrote:

In D60831#1470520, @hfinkel wrote:

How will this appear to profiling tools using PC sampling and using debug info to map the PC samples back to line numbers in the code?

I brought this up in some offline discussions which all concluded that the impact on profilers would be small and the trade off for better debugging is worth it. The impact on profilers seems like it would be small because the middle block is only visited once after running through the vectorized loop.

I am glad you asked because this concern gives rise to an argument for giving the middle block instructions line 0 instead, and i am interested in hearing other's opinions.

Interesting. The middle block just has the check for whether or not we need to run the remainder loop, right? I can definitely see this as kind of latch-like.

@jmellorcrummey , do you have an opinion on this?

Commenting from my phone without a spec (flying without a net): I think we should change the name of the bit ompt_enabled.enabled to ompt_enabled.activated. That would make joachim’s distinction clear. I was commenting on the setting of OMP_TOOL=enabled. There is a difference between enabled and activated. Simon’s commentary in and about the test case muddled the activated vs. enabled distinction.

None of this makes sense. If OMPT is not enabled, there should be no call to the ompt_initializer. For that reason, no tool should get pointers to OMPT runtime entry points if OMPT is not enabled. Therefore, the OMPT runtime entry points don’t need to check if OMPT is enabled.

I hope that you are testing it on a system with weak ordering rather than an Intel chip ;-)

I have a few more comments about the memory orderings. Not having too much order is important for performance. Fences that are inserted unless they are explicitly omitted hurt performance.

Now that I have heard some thoughts from Intel, this patch does the following:

The fix looks good, but the file should have a full diff. Please update with a diff that includes the whole file instead of just the change.

I have a few unsolicited comments to offer:

Remove comment that described ITT as always on.

I am somewhat surprised by this set of changes. The set of thread states being maintained is very close to what OMPT supports. Are the differences worthwhile? If so, why not have someone from Intel participate in the OMPT discussions and lobby for changes. If the differences are not worthwhile, then what is the point of having maintaining two independent implementations of state maintenance that largely duplicate one another? Why not just use the OMPT states? If states aren't being maintained properly for the OpenMP library (I know that not all of the OMPT state maintenance is implemented or correct), why doesn't someone from Intel with intimate knowledge of how the runtime works fix it? If I were to fix the OMPT implementation, I would consider simply putting OMPT state maintenance in the same places that Intel engineers who wrote the library put them (where they are in this patch).

LGTM.

I haven't had the time to analyze this patch in detail. To me, it looks like it fixes a symptom (an incorrect argument passed to a callback) rather than an underlying cause (information about the top task frame may be improperly updated).

Update revision to meet Jonathan's requirement of the day: change control flag to LIBOMP_ENABLE_SHARED.

Address Jonathan's comments:

• Renamed flag LIBOMP_SHARED_LIBRARY to LIBOMP_STATIC_LIBRARY.
• Add documentation for the flag to runtime/Build_With_CMake.txt

LGTM.

LGTM.

Neither this patch nor the original code address the problem that needs attention.

Overall, the patch looks good. Sorry that it has taken me so long to review it.

Looks good to me.

Looks good to me.

Please update your patch in differential to include full diffs with "git diff -U999999 master"

My aim is to keep ompt-general.c completely of details specific to a particular runtime, so its code can be reused in other runtimes. For that reason, I don't support adding an include of kmp_config.h to ompt-general.c.

I apologize that I misunderstood begin/end pair being implemented when I initially reviewed the patch.

It appears that no context is available for the diffs. Without the context, I can't tell whether the additions are appropriate or not. The callbacks are intended to monitor the beginning and end of an episode of waiting. There is no evidence of the waiting in the diffs provided without context.

I have one more minor comment

I was wrong. I had one more thing to say :-).

One final comment:

Two more things:

While the changes to the build system seem fine to me, the changes to ompt-general.c are awkward. I don't like the "#if KMP_HAVE_PSAPI" special casing inside ompt_pre_init. Also, there is no need for a function ompt_aux_tool. I recommend the following instead:

I am fine with the approach proposed below. We can refine the interface at a later date when the lock hints become available.

John Mellor-Crummey Professor
Dept of Computer Science Rice University
email: johnmc@rice.edu phone: 713-348-5179

In reading this patch over again, I realized that you implemented the OMPT TR2 lock initialization callback rather than the OMPT working draft, which also passes a lock hint and a lock type to the init callback. The OMPT implementation in LLVM OpenMP is evolving with the current version of the OMPT draft. There are features already in the code that aren't in OMPT TR2. Fort that reason, I think you should add the lock hint and lock type parameters.

The lock init and acquired callbacks are conditionally included according to #if OMPT_SUPPORT && OMPT_BLAME

LGTM. Thanks for finding and correcting this performance problem.

Addressed comments:

The problem is not the OMPT callbacks; the problem was the state in which they were reached.

The change looks OK. If it is being made to isolate changes needed only by OMPT, then fine. If it is being made because there is concern that passing an extra argument to this routine might be a performance issue, then I suggest that there are bigger fish to fry than this.

LGTM.

Is there any reason why this hasn't been committed yet?

Address concerns about indentation.

A prior patch changed the name of OMPT placeholder functions to move them from the omp_ name space to the ompt_ name space. This patch moves the names of the types of these functions into the OMPT name space as well.

While I don't object to this commit, I'll note that what we hope will be a standard for an OpenMP debugging interface known as OMPD is under active development. The current draft is posted here: https://github.com/OpenMPToolsInterface/OMPD-Technical-Report

Moved OMPT placeholders from omp namespace to ompt namespace.

Remove use of assignment to multiple struct fields using .fieldname syntax. This doesn't work with gcc 4.8 and earlier.
Replace with elementwise field assignments.

Patch to correct handling of OMPT states and implicit task ids. I'm a newbie committer to LLVM, so any comments about how to accelerate getting this commit accepted would be appreciated.

I am adding a comment to trigger reposting of this patch. I forgot to join openmp-commits before submitting this patch.