Page MenuHomePhabricator

KMP_HW_SUBSET extended with NUMA support when HWLOC enabled
ClosedPublic

Authored by AndreyChurbanov on Apr 3 2017, 7:37 AM.

Details

Summary
  • KMP_HW_SUBSET parsing rewritten;
  • Added support for L2 cache and NUMA nodes, given the HWLOC enabled;
  • old functionality just reformatted with no functional changes.

Diff Detail

Repository
rL LLVM

Event Timeline

AndreyChurbanov created this revision.Apr 3 2017, 7:37 AM
jlpeyton accepted this revision.Apr 5 2017, 2:37 PM

LGTM

This revision is now accepted and ready to land.Apr 5 2017, 2:37 PM
This revision was automatically updated to reflect the committed changes.
pawosm01 reopened this revision.Apr 24 2017, 10:18 AM
pawosm01 added a subscriber: pawosm01.

Due to use of new enum constants (HWLOC_OBJ_NUMANODE, HWLOC_OBJ_PACKAGE) this code does not compile on most of the mainstream Linux distributions where pre 2.0 hwloc (-devel) is still provided:

runtime/src/kmp_affinity.cpp:3504:47: error: ‘HWLOC_OBJ_NUMANODE’ was not declared in this scope
       hN = hwloc_get_ancestor_obj_by_type(tp, HWLOC_OBJ_NUMANODE, hT);

runtime/src/kmp_affinity.cpp:3505:47: error: ‘HWLOC_OBJ_PACKAGE’ was not declared in this scope
       hS = hwloc_get_ancestor_obj_by_type(tp, HWLOC_OBJ_PACKAGE, hT);
This revision is now accepted and ready to land.Apr 24 2017, 10:18 AM
pawosm01 requested changes to this revision.Apr 24 2017, 10:20 AM
This revision now requires changes to proceed.Apr 24 2017, 10:20 AM

what's the next step here?

what's the next step here?

I'd vote for closing this request (maybe approving it first). The problem with this patch you raised was fixed by the commit rL301349 (D32496).

An alternative would be to revert both rL300220 and rL301349, merge them into single patch, and update the D31600 with this patch.

Anyway, it does not look convenient to resolve the problem in already committed big patch by reverting it, fixing, etc., comparing to just apply the new small patch.

So between the two ways:

(1) reopen the review request that has problem, revert the corresponding commit, update the patch with fixed version, review, commit, close the old request;
(2) open bugzilla request, open new request with the fix, review, commit, close the new request.

I'd prefer to go the second way, unless there is policy that requires to go the fist one (I haven't found the policy anywhere so far).

Maybe others have something to comment here, this is the second time we have problem with what to do with tiny fix of the problem in big already committed patch applied as a separate patch, as opposed to revert commit + merge the fix + reapply fixed patch.

I'd vote for closing this request (maybe approving it first). The problem with this patch you raised was fixed by the commit rL301349 (D32496).

If trunk works, there is nothing to do. The fix has been committed relatively close after the initial patch, so all release branches work.

Maybe others have something to comment here, this is the second time we have problem with what to do with tiny fix of the problem in big already committed patch applied as a separate patch, as opposed to revert commit + merge the fix + reapply fixed patch.

Committing fixes for previous patches shouldn't require to revert the complete change first. This is normally done in LLVM/Clang repositories when the problem impacts many people and / or some time is needed to uncover and fix the problem.

Paul,

I guess only you can change the "Needs Revision" status of the patch, so that we could close it.

pawosm01 accepted this revision.Jan 8 2018, 11:56 AM
This revision is now accepted and ready to land.Jan 8 2018, 11:56 AM