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

Remove gstats_label_ifs to avoid name clash #48

Merged
merged 2 commits into from
Dec 1, 2023

Conversation

reuterbal
Copy link
Contributor

When linking IFS against ectrans, we have a symbol clash and this one takes precedence. Since gstat labels are set-up locally in the benchmark driver, I don't think this file needs to be there in the first place.

Currently confirming this doesn't cause problems on LUMI. Will file the same change for redgreengpu.

@samhatfield
Copy link
Collaborator

Agreed. I believe this file is only there to provide additional labels for the benchmark program that aren't covered by its own labelling routine.

@reuterbal
Copy link
Contributor Author

Yes indeed, but it's not actually called, so not fulfilling that purpose afaict - are there any labels missing from this to the best of your knowledge?

@samhatfield
Copy link
Collaborator

Good point! And there is no corresponding file in src/trans/cpu/internal in any case. Must have been added during debug or something.

You mean labels missing from gstats_label_ifs.F90?

@reuterbal
Copy link
Contributor Author

Both directions actually, but for the sake of this PR I was wondering if there were labels missing in the driver.

@samhatfield
Copy link
Collaborator

Oh yes, many!:

  • 104
  • 105
  • 132
  • 133
  • 157
  • 180
  • 181
  • 182
  • 183
  • 783
  • 785
  • 786
  • 798
  • 801
  • 803
  • 804
  • 805
  • 806
  • 807
  • 809
  • 810
  • 811
  • 812
  • 814
  • 815
  • 851
  • 1141
  • 1251
  • 1252
  • 1253
  • 1429
  • 1601
  • 1604
  • 1607
  • 1608
  • 1639
  • 1640
  • 1641
  • 1642
  • 1643
  • 1644
  • 1645
  • 1646
  • 1647
  • 1648
  • 1650
  • 1651
  • 1663
  • 1801
  • 1804
  • 1805
  • 1806
  • 1807
  • 1808
  • 1809
  • 1810
  • 1802
  • 1901

And that's probably why someone just copied the routine from the IFS (even if they stopped using it at some point).

@lukasm91
Copy link
Collaborator

Hope it's fine if I also have a comment here: I might at least have added some gstats labels e.g. in the GPU branches, already back in the early OpenACC version. But wouldn't it make sense to just put the ectrans labels into ectrans, and probably reserve a range for ectrans?
Otherwise it's going to be difficult keeping the IFS gstats_label_ifs compatible with ectrans (and all ectrans branches will always require the IFS compatible version). It doesn't really make much sense if internals of ectrans (the label names) leak into the IFS codes. IFS can then just call ectrans_set_labels (and maybe gstats_psut to distribute the labels) once and will always get the right labels. It can even take an optional range where it can put its labels.

@samhatfield
Copy link
Collaborator

Perhaps GSTATS_LABEL_IFS could be kept in gpu/internal, added to cpu/internal, renamed GSTATS_LABEL_ECTRANS and called from SETUP_TRANS0?

@samhatfield
Copy link
Collaborator

samhatfield commented Nov 29, 2023

But it might then be necessary to call GSTATS_SETUP always before SETUP_TRANS0, which is a bit awkward.

@reuterbal
Copy link
Contributor Author

I fully agree that libraries should manage their gstats labels - and various callback formats could be envisaged how and when this should happen and where a handshake exchanges valid label ranges and warns if there's an "out-of-bounds" label in the current library. But I would argue this should be a separate PR.

@wdeconinck
Copy link
Collaborator

I agree completely with @lukasm91
I also would be happy to have some kind of dynamic registration with looking up free UIDS, but that comes at a runtime cost, depending on the location it is called. Typically this is not in a tight loop though.

Food for thought for future PR.

@ioanhadade
Copy link
Contributor

Dynamic UIDs can bring confusion further down the line when you post process events as in some cases we do rely on the fact that labels do not change in meaning. Also when we present certain data (e.g., detailed stats including communication and barriers).

@wdeconinck wdeconinck merged commit 888e029 into amdgpu Dec 1, 2023
20 checks passed
@reuterbal reuterbal deleted the nabr-amdgpu-no-gstats-label-ifs branch January 9, 2024 10:21
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.

5 participants