This is an archive of the discontinued LLVM Phabricator instance.

[OMPT] extend ompt tests by checks for frame pointers
ClosedPublic

Authored by protze.joachim on Aug 9 2016, 3:37 AM.

Details

Summary

OMPT tests can check for right frame information of tasks:

  • parent_task_frame was directly printed as a pointer, but actually points to a struct ompt_frame {void*, void*}
  • NULL is printed in the beginning of execution and loaded to FileChecker variable [[NULL]]
  • implicit tasks now also print their frame information
  • macro to print frame address from application
  • print task info for barrier begin

Diff Detail

Event Timeline

protze.joachim retitled this revision from to [OMPT] extend ompt tests by checks for frame pointers.
protze.joachim updated this object.
jlpeyton accepted this revision.Aug 17 2016, 12:58 PM
jlpeyton edited edge metadata.

LGTM. It would be good if Jonas Hahnfeld or John Mellor-Crummey could review this series of OMPT changes as well since they are more familiar with it.

This revision is now accepted and ready to land.Aug 17 2016, 12:58 PM

Adding Jonas and John to reviewers.

Hahnfeld edited edge metadata.Aug 18 2016, 6:25 AM

LGTM. It would be good if Jonas Hahnfeld or John Mellor-Crummey could review this series of OMPT changes as well since they are more familiar with it.

This might be a more general question and I'll be happy reposting it on openmp-dev to get broader feedback if we want to further discuss it:

Should we review patches that come from the same team in the same company / university?
Ideally I would prefer no and that's why I told Joachim to not put me as reviewer because we already discussed some of the changes and their implementation offline.

(However I understand that this is different for Intel: If I remember correctly there are multiple teams in multiple locations and you probably have more internal review than we have for some research coding...)

I'm adding Olga as well as she is now part of the OMPT subcommittee.

Should we review patches that come from the same team in the same company / university?
Ideally I would prefer no and that's why I told Joachim to not put me as reviewer because we already discussed some of the changes and their implementation offline.

I think it would be good to have a final stamp of approval from someone intimate with this component of the code base / future OpenMP specification. There are not many regular reviewers to go around for OpenMP code (especially external to Intel), and even if you've already had an internal discussion within your group about the patches, giving it the ok at least partially acknowledges that activity. I also agree that one of us, Intel or anyone else involved with this project, can look at these patches so there is external input. The more eyes that have seen the patch and acknowledge it by writing a comment on here, the better.

That is of course just my opinion. Anyone else want to chime in?

Hahnfeld resigned from this revision.Aug 19 2016, 12:09 AM
Hahnfeld removed a reviewer: Hahnfeld.

I think it would be good to have a final stamp of approval from someone intimate with this component of the code base / future OpenMP specification. There are not many regular reviewers to go around for OpenMP code (especially external to Intel), and even if you've already had an internal discussion within your group about the patches, giving it the ok at least partially acknowledges that activity. I also agree that one of us, Intel or anyone else involved with this project, can look at these patches so there is external input. The more eyes that have seen the patch and acknowledge it by writing a comment on here, the better.

As a proposal, I will put an LGTM and "Resign as Reviewer" to make clear that the patch should in my opinion receive additional (external) review

This revision was automatically updated to reflect the committed changes.