Page MenuHomePhabricator

jmellorcrummey (John Mellor-Crummey)
User

Projects

User does not belong to any projects.

User Details

User Since
Jun 25 2015, 7:58 PM (212 w, 3 d)

Recent Activity

Apr 18 2019

jmellorcrummey added a comment to D60831: [DebugInfo@O2][LoopVectorize] pr39024: Vectorized code linenos step through loop even after completion.

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.

Apr 18 2019, 9:39 PM · debug-info, Restricted Project
jmellorcrummey added a comment to D60831: [DebugInfo@O2][LoopVectorize] pr39024: Vectorized code linenos step through loop even after completion.

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.

Apr 18 2019, 7:55 PM · debug-info, Restricted Project
jmellorcrummey added a comment to D60831: [DebugInfo@O2][LoopVectorize] pr39024: Vectorized code linenos step through loop even after completion.

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.

Apr 18 2019, 4:39 PM · debug-info, Restricted Project
jmellorcrummey added a comment to D60831: [DebugInfo@O2][LoopVectorize] pr39024: Vectorized code linenos step through loop even after completion.

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?

Apr 18 2019, 2:28 PM · debug-info, Restricted Project

Feb 6 2019

jmellorcrummey added a reviewer for D57469: Set exit_frame in master task before calling a user's outlined function for a parallel loop construct: AndreyChurbanov.
Feb 6 2019, 9:09 AM · Restricted Project

Jan 30 2019

jmellorcrummey updated the summary of D57469: Set exit_frame in master task before calling a user's outlined function for a parallel loop construct.
Jan 30 2019, 3:29 PM · Restricted Project
jmellorcrummey created D57469: Set exit_frame in master task before calling a user's outlined function for a parallel loop construct.
Jan 30 2019, 1:02 PM · Restricted Project

Jun 18 2018

jmellorcrummey added a comment to D47717: [OMPT] Make sure that OMPT is enabled in runtime entry points that access internals of the runtime.

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.

Jun 18 2018, 10:53 AM
jmellorcrummey requested changes to D47717: [OMPT] Make sure that OMPT is enabled in runtime entry points that access internals of the runtime.

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.

Jun 18 2018, 4:53 AM

May 24 2016

jmellorcrummey added a comment to D19878: Use C++11 atomics for ticket locks implementation.

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

May 24 2016, 9:37 AM
jmellorcrummey added a comment to D19878: Use C++11 atomics for ticket locks implementation.

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 24 2016, 6:59 AM

May 23 2016

jmellorcrummey updated the diff for D20517: Make LIBOMP_USE_ITT_NOTIFY a setting that can be enabled or disabled.

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

May 23 2016, 3:46 PM
jmellorcrummey added a comment to D20525: [OMPT] Use more general function for getting gtid.

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.

May 23 2016, 8:29 AM
jmellorcrummey added a comment to D19878: Use C++11 atomics for ticket locks implementation.

I have a few unsolicited comments to offer:

May 23 2016, 7:58 AM

May 22 2016

jmellorcrummey updated the diff for D20517: Make LIBOMP_USE_ITT_NOTIFY a setting that can be enabled or disabled.

Remove comment that described ITT as always on.

May 22 2016, 10:04 PM
jmellorcrummey retitled D20517: Make LIBOMP_USE_ITT_NOTIFY a setting that can be enabled or disabled from to Make LIBOMP_USE_ITT_NOTIFY a setting that can be enabled or disabled.
May 22 2016, 10:01 PM

Apr 28 2016

jmellorcrummey added a comment to D19229: [STATS] Use partitioned timer scheme.

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).

Apr 28 2016, 11:51 AM

Feb 12 2016

jmellorcrummey added a comment to D17145: [OMPT] Frame information for openmp taskwait.

LGTM.

Feb 12 2016, 7:22 AM

Feb 4 2016

jmellorcrummey added a comment to D16714: [OMPT] Fix wrong parent_task_id in serialized parallel_begin with GCC.

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).

Feb 4 2016, 2:13 PM
jmellorcrummey updated the diff for D16525: Add CMAKE variable to enable creation of static OpenMP libraries instead of dynamic ones.

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

Feb 4 2016, 10:49 AM

Feb 3 2016

jmellorcrummey updated the diff for D16525: Add CMAKE variable to enable creation of static OpenMP libraries instead of dynamic ones.

Address Jonathan's comments:

  • Renamed flag LIBOMP_SHARED_LIBRARY to LIBOMP_STATIC_LIBRARY.
  • Add documentation for the flag to runtime/Build_With_CMake.txt
Feb 3 2016, 11:54 PM

Jan 24 2016

jmellorcrummey added a comment to D16347: [OMPT]: Fix the order of implicit_task_end_events.

LGTM.

Jan 24 2016, 12:30 PM
jmellorcrummey added a comment to D14746: Add support for ompt_event_task_dependences and ompt_event_task_dependence_pair.

LGTM.

Jan 24 2016, 12:24 PM
jmellorcrummey retitled D16525: Add CMAKE variable to enable creation of static OpenMP libraries instead of dynamic ones from to Add CMAKE variable to enable creation of static OpenMP libraries instead of dynamic ones.
Jan 24 2016, 12:15 PM

Dec 22 2015

jmellorcrummey added a comment to D14544: [OMPT] Fix the lock id provided for ordered events.

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

Dec 22 2015, 3:10 PM
jmellorcrummey added a reviewer for D14746: Add support for ompt_event_task_dependences and ompt_event_task_dependence_pair: jlpeyton.
Dec 22 2015, 11:34 AM
jmellorcrummey added a comment to D14746: Add support for ompt_event_task_dependences and ompt_event_task_dependence_pair.

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

Dec 22 2015, 11:33 AM

Dec 18 2015

jmellorcrummey retitled D15656: Fix build error: OMPT_SUPPORT=true was not tested after locking changes were made from to Fix build error: OMPT_SUPPORT=true was not tested after locking changes were made.
Dec 18 2015, 3:48 PM

Nov 11 2015

jmellorcrummey updated subscribers of D14565: Fix OMPT_TRACE/OMPT_BLAME definition inheritance (was: Missing include kmp_config.h).

Looks good to me.

Nov 11 2015, 8:22 AM
jmellorcrummey updated subscribers of D14566: Add ompt_event_task_switch event into OMPT/OpenMP.

Looks good to me.

Nov 11 2015, 8:20 AM
jmellorcrummey added a comment to D14566: Add ompt_event_task_switch event into OMPT/OpenMP.

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

Nov 11 2015, 6:24 AM
jmellorcrummey added a comment to D14565: Fix OMPT_TRACE/OMPT_BLAME definition inheritance (was: Missing include kmp_config.h).

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 11 2015, 6:12 AM

Nov 9 2015

jmellorcrummey added a comment to D14383: Add OMPT events for the OpenMP taskwait construct..

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

Nov 9 2015, 7:15 AM

Nov 6 2015

jmellorcrummey added a comment to D14383: Add OMPT events for the OpenMP taskwait construct..

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 6 2015, 1:33 PM

Nov 4 2015

jmellorcrummey retitled D14355: Improve OMPT initialization code from to Improve OMPT initialization code.
Nov 4 2015, 3:52 PM

Oct 29 2015

jmellorcrummey added a comment to D14027: [OMPT] Windows Support for OMPT.

I have one more minor comment

Oct 29 2015, 1:22 PM

Oct 23 2015

jmellorcrummey added a comment to D14027: [OMPT] Windows Support for OMPT.

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

Oct 23 2015, 2:53 PM
jmellorcrummey added a comment to D14027: [OMPT] Windows Support for OMPT.

One final comment:

Oct 23 2015, 2:42 PM
jmellorcrummey added a comment to D14027: [OMPT] Windows Support for OMPT.

Two more things:

Oct 23 2015, 2:21 PM
jmellorcrummey added a comment to D14027: [OMPT] Windows Support for OMPT.

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 23 2015, 2:13 PM

Oct 13 2015

jmellorcrummey added a comment to D13689: Add OMPT events for API locking.

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

Oct 13 2015, 10:17 AM
jmellorcrummey added a comment to D13689: Add OMPT events for API locking.

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.

Oct 13 2015, 7:10 AM
jmellorcrummey added a comment to D13689: Add OMPT events for API locking.

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

Oct 13 2015, 6:49 AM

Oct 7 2015

jmellorcrummey retitled D13502: Reduce overhead of OMPT from to Reduce overhead of OMPT.
Oct 7 2015, 2:40 AM
jmellorcrummey added a comment to D13494: [OMPT] Initialize task fields only if needed.

LGTM. Thanks for finding and correcting this performance problem.

Oct 7 2015, 2:35 AM

Sep 21 2015

jmellorcrummey updated the diff for D12998: Overhaul OMPT initialization interface.

Addressed comments:

Sep 21 2015, 9:57 AM

Sep 19 2015

jmellorcrummey added a reviewer for D12998: Overhaul OMPT initialization interface: jlpeyton.
Sep 19 2015, 8:43 PM
jmellorcrummey added a reviewer for D12999: Simplify control variable logic for OMPT: jlpeyton.
Sep 19 2015, 8:42 PM
jmellorcrummey retitled D12999: Simplify control variable logic for OMPT from to Simplify control variable logic for OMPT.
Sep 19 2015, 8:42 PM
jmellorcrummey retitled D12998: Overhaul OMPT initialization interface from to Overhaul OMPT initialization interface.
Sep 19 2015, 4:16 PM

Sep 16 2015

jmellorcrummey retitled D12911: Correct an incorrect OMPT ifdef from to Correct an incorrect OMPT ifdef .
Sep 16 2015, 12:25 PM

Sep 10 2015

jmellorcrummey added a comment to D12754: Fix assertion that arises when waiting for proxy tasks on runtime shutdown.

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

Sep 10 2015, 3:50 PM

Aug 31 2015

jmellorcrummey added a comment to D12495: Remove fork_context argument from __kmp_join_call() when OMPT is off.

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 31 2015, 10:51 AM

Aug 25 2015

jmellorcrummey added a comment to D12331: Removing the Makefile+Perl build system.

LGTM.

Aug 25 2015, 1:22 PM

Jul 23 2015

jmellorcrummey added a comment to D11269: Patch out an (apparently unwarranted) fatal assertion in OpenMP runtime until preconditions are met.

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

Jul 23 2015, 8:53 AM

Jul 16 2015

jmellorcrummey retitled D11269: Patch out an (apparently unwarranted) fatal assertion in OpenMP runtime until preconditions are met from to Patch out an (apparently unwarranted) fatal assertion in OpenMP runtime until preconditions are met.
Jul 16 2015, 8:43 AM
jmellorcrummey updated the diff for D11259: Fix OMPT support for task frames for parallel regions and parallel regions + loops.

Address concerns about indentation.

Jul 16 2015, 8:01 AM
jmellorcrummey added inline comments to D11259: Fix OMPT support for task frames for parallel regions and parallel regions + loops.
Jul 16 2015, 6:54 AM
jmellorcrummey retitled D11259: Fix OMPT support for task frames for parallel regions and parallel regions + loops from to Fix OMPT support for task frames for parallel regions and parallel regions + loops.
Jul 16 2015, 6:04 AM

Jul 13 2015

jmellorcrummey added a comment to D11171: Rename OMPT placeholder type names to be in the OMPT name space.

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 13 2015, 6:03 PM
jmellorcrummey retitled D11171: Rename OMPT placeholder type names to be in the OMPT name space from to Rename OMPT placeholder type names to be in the OMPT name space.
Jul 13 2015, 6:02 PM

Jul 9 2015

jmellorcrummey added a comment to D10038: Enable debugger support.

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

Jul 9 2015, 11:27 AM
jmellorcrummey set the repository for D11062: Bug fixes in OMPT support to rL LLVM.
Jul 9 2015, 9:10 AM
jmellorcrummey updated the diff for D11062: Bug fixes in OMPT support.

Moved OMPT placeholders from omp namespace to ompt namespace.

Jul 9 2015, 8:58 AM
jmellorcrummey retitled D11062: Bug fixes in OMPT support from to Bug fixes in OMPT support.
Jul 9 2015, 7:38 AM

Jun 29 2015

jmellorcrummey added a comment to D10798: Improve OMPT code: remove use of assignment to multiple struct fields using .fieldname .

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 29 2015, 1:32 AM
jmellorcrummey retitled D10798: Improve OMPT code: remove use of assignment to multiple struct fields using .fieldname from to Improve OMPT code: remove use of assignment to multiple struct fields using .fieldname .
Jun 29 2015, 1:32 AM

Jun 27 2015

jmellorcrummey added a comment to D10759: Fix OMPT state maintenance for barriers in OpenMP runtime. Fix missing initialization of implicit task id..

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 27 2015, 2:22 PM
jmellorcrummey set the repository for D10759: Fix OMPT state maintenance for barriers in OpenMP runtime. Fix missing initialization of implicit task id. to rL LLVM.
Jun 27 2015, 2:18 PM
jmellorcrummey retitled D10759: Fix OMPT state maintenance for barriers in OpenMP runtime. Fix missing initialization of implicit task id. from Fix OMPT state maintenance for barriers in OpenMP runtime to Fix OMPT state maintenance for barriers in OpenMP runtime. Fix missing initialization of implicit task id..
Jun 27 2015, 2:17 PM
jmellorcrummey updated the diff for D10759: Fix OMPT state maintenance for barriers in OpenMP runtime. Fix missing initialization of implicit task id..
Jun 27 2015, 2:16 PM

Jun 26 2015

jmellorcrummey added a comment to D10759: Fix OMPT state maintenance for barriers in OpenMP runtime. Fix missing initialization of implicit task id..

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

Jun 26 2015, 5:47 AM

Jun 25 2015

jmellorcrummey added a reviewer for D10759: Fix OMPT state maintenance for barriers in OpenMP runtime. Fix missing initialization of implicit task id.: jlpeyton.
Jun 25 2015, 8:38 PM
jmellorcrummey updated subscribers of D10759: Fix OMPT state maintenance for barriers in OpenMP runtime. Fix missing initialization of implicit task id..
Jun 25 2015, 8:27 PM
jmellorcrummey retitled D10759: Fix OMPT state maintenance for barriers in OpenMP runtime. Fix missing initialization of implicit task id. from to Fix OMPT state maintenance for barriers in OpenMP runtime.
Jun 25 2015, 8:22 PM