- User Since
- Jun 25 2015, 7:58 PM (221 w, 1 d)
Apr 18 2019
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.
Feb 6 2019
Jan 30 2019
Jun 18 2018
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.
May 24 2016
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.
May 23 2016
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:
May 22 2016
Remove comment that described ITT as always on.
Apr 28 2016
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).
Feb 12 2016
Feb 4 2016
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.
Feb 3 2016
Address Jonathan's comments:
- Renamed flag LIBOMP_SHARED_LIBRARY to LIBOMP_STATIC_LIBRARY.
- Add documentation for the flag to runtime/Build_With_CMake.txt
Jan 24 2016
Dec 22 2015
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.
Dec 18 2015
Nov 11 2015
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.
Nov 9 2015
I apologize that I misunderstood begin/end pair being implemented when I initially reviewed the patch.
Nov 6 2015
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.
Nov 4 2015
Oct 29 2015
I have one more minor comment
Oct 23 2015
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:
Oct 13 2015
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: firstname.lastname@example.org 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
Oct 7 2015
LGTM. Thanks for finding and correcting this performance problem.
Sep 21 2015
Sep 19 2015
Sep 16 2015
Sep 10 2015
The problem is not the OMPT callbacks; the problem was the state in which they were reached.
Aug 31 2015
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.
Aug 25 2015
Jul 23 2015
Is there any reason why this hasn't been committed yet?
Jul 16 2015
Address concerns about indentation.
Jul 13 2015
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.
Jul 9 2015
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.
Jun 29 2015
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.
Jun 27 2015
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.
Jun 26 2015
I am adding a comment to trigger reposting of this patch. I forgot to join openmp-commits before submitting this patch.