Page MenuHomePhabricator

[OMPT] Rename ompt_frame_t to omp_frame_t
ClosedPublic

Authored by sconvent on Feb 21 2018, 5:07 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

sconvent created this revision.Feb 21 2018, 5:07 AM
sconvent edited the summary of this revision. (Show Details)

Do we really need this change?
I found a comment from John M-C at https://github.com/OpenMPToolsInterface/OpenMP-4.5-spec/commit/7bed0e0e6b96e0cfa6d964de2101c2172a0a47e7#commitcomment-24561958 (that commit replaced ompt_frame_t to omp_frame_t in the Spec):

This change needs to be reverted. We had originally planned to use omp_frame_t, but now are using ompt_frame_t because it turns out that we can't share the type: OMPD needs ompd_address_t instead of void * for the addresses.

runtime/src/include/50/ompt.h.var
302 ↗(On Diff #135233)

Add space before /*

344 ↗(On Diff #135233)

Add space before /*

sconvent updated this revision to Diff 135241.Feb 21 2018, 5:56 AM

Fix indentation

As far as I know, the current implementation is very close to the specification in TR6.
Here we have a tiny diff between the spec and the implementation. I agree that we should not apply this patch if we will roll back the change in the spec.

So I propose to discuss this in the Tools WG first and then decide whether to apply the patch. I will schedule this for the next call.

protze.joachim accepted this revision.May 7 2018, 9:28 AM

Result of discussion in Tools WG was that the spec will keep omp_frame_t. I will submit this patch together with D43568

This revision is now accepted and ready to land.May 7 2018, 9:30 AM
Hahnfeld accepted this revision.May 8 2018, 12:23 AM

Ok, let's do this.

Closed by commit rL333367: [OMPT] Rename ompt_frame_t to omp_frame_t (authored by jprotze, committed by ). · Explain WhyMay 28 2018, 1:19 AM
This revision was automatically updated to reflect the committed changes.