This is an archive of the discontinued LLVM Phabricator instance.

[OMPT] Fix ompt_get_task_info() and add tests for it
ClosedPublic

Authored by sconvent on Jan 24 2018, 5:46 AM.

Details

Summary

The thread_num parameter of ompt_get_task_info() was not being used previously, but need to be set.

The print_task_type() function (form the task-types.c testcase) was merged into the print_ids() function (in callback.h). Testing of ompt_get_task_info() was added to the task-types.c testcase. It was not tested extensively previously.

Diff Detail

Repository
rL LLVM

Event Timeline

sconvent created this revision.Jan 24 2018, 5:46 AM
sconvent updated this revision to Diff 131253.Jan 24 2018, 6:26 AM

Merge D42470 into this revision

Hahnfeld requested changes to this revision.Jan 25 2018, 1:50 AM

You need to update title and summary to reflect the latest changes and give some reasons.

runtime/test/ompt/callback.h
74 ↗(On Diff #131253)

I know we have been doing a poor job in the past, but this line is almost unreadable. Maybe we should start formatting all changes to tests with clang-format as well...

This revision now requires changes to proceed.Jan 25 2018, 1:50 AM
protze.joachim added inline comments.Jan 25 2018, 2:10 AM
runtime/test/ompt/callback.h
74 ↗(On Diff #131253)

The problem with clang-format is, that it breaks the FileCheck patterns by enforcing line breaks for the comments

Hahnfeld added inline comments.Jan 25 2018, 2:12 AM
runtime/test/ompt/callback.h
74 ↗(On Diff #131253)

You can use CHECK-SAME for that.

Hahnfeld added inline comments.Jan 25 2018, 2:26 AM
runtime/test/ompt/callback.h
74 ↗(On Diff #131253)

(That's why I only suggest it for new tests, I don't mean to update all existing test cases)

My point is that LLVM uses clang-format for a reason which is a uniform code style that is reasonably easy to read and understand.

sconvent retitled this revision from [OMPT] Also test ompt_get_task_info() in task_types.c testcase to [OMPT] Fix ompt_get_task_info() and add tests for it.Jan 25 2018, 4:03 AM
sconvent edited the summary of this revision. (Show Details)
Hahnfeld resigned from this revision.Feb 12 2018, 3:39 AM
sconvent updated this revision to Diff 134216.Feb 14 2018, 6:10 AM

Check task_ids in task_types.c testcase

sconvent updated this revision to Diff 135394.Feb 22 2018, 3:59 AM

Update Formatting

sconvent updated this revision to Diff 135395.Feb 22 2018, 4:01 AM

Fix small copy-and-paste mistake

You should probably explain why each task is in its own single region and why that works as discussed internally.

runtime/test/ompt/callback.h
36 ↗(On Diff #135395)

This function is not formatted.

73–75 ↗(On Diff #135395)

Same

sconvent updated this revision to Diff 136240.Feb 28 2018, 12:46 AM

Reformat using git-clang-format
Applying clang-format to callback.h does not improve readability

Hahnfeld accepted this revision.Feb 28 2018, 9:15 AM

LGTM

runtime/test/ompt/tasks/task_types.c
7–8 ↗(On Diff #136240)

Please reformat this test on commit, this should fix some inconsistencies.

This revision is now accepted and ready to land.Feb 28 2018, 9:15 AM
This revision was automatically updated to reflect the committed changes.