Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Integrate Caliper #331

Conversation

michaelmckinsey1
Copy link
Contributor

@michaelmckinsey1 michaelmckinsey1 commented Jun 8, 2023

Caliper Integration for RAJAPerf

This work is a variant of #254

  • This PR is a feature.
  • It does the following:
    • Implements caliper annotations into RAJAPerf.
    • Adds new Adiak metadata fields.
  • This feature is at the request of the PAVE team.

This is dependent on #333 #335

@michaelmckinsey1 michaelmckinsey1 changed the title Init Integrate Caliper Jun 8, 2023
@michaelmckinsey1 michaelmckinsey1 force-pushed the feature/MichaelMcKinsey1/caliper-integration branch 5 times, most recently from 6ce1c05 to 8318ee1 Compare June 15, 2023 00:09
@michaelmckinsey1
Copy link
Contributor Author

michaelmckinsey1 commented Jun 15, 2023

Ignore the tuning commits, they will go away once #333 is merged merged

@michaelmckinsey1 michaelmckinsey1 force-pushed the feature/MichaelMcKinsey1/caliper-integration branch from 8573ef9 to 7c14475 Compare June 23, 2023 18:21
@@ -308,6 +341,7 @@ class KernelBase
MPI_Barrier(MPI_COMM_WORLD);
#endif
timer.start();
CALI_START;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put CALI_START after and CALI_STOP before the existing timers. Is this acceptable? I assume if someone turns caliper on they would be looking at those timings so they should be more accurate and in the opposite case caliper would be off so it shouldn't impact the original timers that much. Not sure how much impact this has.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that choice makes most sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Caliper is off, do the macros become no-ops with a simple run time conditional evaluation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. If RAJAPerf is not built with caliper, RAJA_PERFSUITE_USE_CALIPER=OFF, the macros will be defined as here, so the only operation will be the #if defined(RAJA_PERFSUITE_USE_CALIPER).

@michaelmckinsey1
Copy link
Contributor Author

michaelmckinsey1 commented Jun 24, 2023

fc53a26 For the doc files I changed as little wording as possible for now. It may be necessary to have stronger wording in regards to running 1 variant & 1 tuning at a time, since multiple tunings at a time makes less sense as we removed the variant.tuning node level from the caliper annotations. It also may be a good idea to enforce (in the code) single variant/tuning or file splitting when caliper is turned on.

@michaelmckinsey1 michaelmckinsey1 force-pushed the feature/MichaelMcKinsey1/caliper-integration branch from 41b7683 to fec89f1 Compare June 29, 2023 20:20
std::string gstr = getGroupName(kstr); \
std::string vstr = "RAJAPerf"; \
CaliMeta(); \
CALI_MARK_BEGIN(vstr.c_str()); \
Copy link
Member

@rhornung67 rhornung67 Jul 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look right to me. Is 'vstr' the "variant" name? If so, then it seems like it should be set by calling getVariantName(), which takes a VariantID enum value.

Am I misunderstanding this?

Also, it feels like we can set up some special utility methods to avoid having to re-parse strings to extract substrings repeatedly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We specficially moved away from getVariantName() to a static string, RAJAPerf, so different runs will not have different top-level node names. This fits the standard expected by our tools. This information should instead be collected in the metadata, as shown here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'm still confused about this. I can ask you about it later sometime.

@michaelmckinsey1 michaelmckinsey1 mentioned this pull request Jul 11, 2023
24 tasks
@michaelmckinsey1
Copy link
Contributor Author

fc53a26 For the doc files I changed as little wording as possible for now. It may be necessary to have stronger wording in regards to running 1 variant & 1 tuning at a time, since multiple tunings at a time makes less sense as we removed the variant.tuning node level from the caliper annotations. It also may be a good idea to enforce (in the code) single variant/tuning or file splitting when caliper is turned on.

Addressed in c0197bf. Generate a single cali file per variant, tuning combo.

@michaelmckinsey1 michaelmckinsey1 marked this pull request as ready for review July 14, 2023 22:56
@michaelmckinsey1 michaelmckinsey1 force-pushed the feature/MichaelMcKinsey1/caliper-integration branch from 04d1e62 to d012525 Compare July 24, 2023 19:52
@michaelmckinsey1
Copy link
Contributor Author

New commits on #352

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants