This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Account for the possibility of multiple target regions per line
ClosedPublic

Authored by mikerice on Sep 28 2022, 10:36 AM.

Details

Summary

Some uses of the preprocessor can result in multiple target regions on the
same line. Cases such as those in the associated lit tests, can now be
supported.

This adds a 'Count' field to TargetRegionEntryInfo to differentiate
regions with the same source position.

The OffloadEntriesInfoManager routines are updated to maintain a count of
regions seen at a location. The registration of regions proceeds that same as
before, but now the next available count is always determined and used in the
offload entry.

Fixes: https://github.com/llvm/llvm-project/issues/52707

Diff Detail

Event Timeline

mikerice created this revision.Sep 28 2022, 10:36 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 28 2022, 10:36 AM
mikerice requested review of this revision.Sep 28 2022, 10:36 AM
mikerice updated this revision to Diff 467825.Oct 14 2022, 10:29 AM
mikerice edited the summary of this revision. (Show Details)
mikerice added a reviewer: ABataev.

rebase

Did you try to call SM.getPresumedLoc(Loc, false); in getTargetEntryUniqueInfo instead?

I think this makes sense, it just reminded me of the dense map nesting mess we have here. I never finished my patch to move this code and clean it up: https://github.com/jdoerfert/llvm-project/commit/cacae2849597a0b0ba9e3dd541467308bd6ef68d (@jsjodin, maybe you are interested in picking up my cleanup+move patch and integrating the count stuff of this patch as well?)

Did you try to call SM.getPresumedLoc(Loc, false); in getTargetEntryUniqueInfo instead?

We thought about using preprocessed line numbers. I think getPresumedLoc would only change the line numbers if there were #line directives in the file.

Tried it with the multiple_regions_per_line.cpp test, we still get 2 regions instead of 4:

!0 = !{i32 0, i32 53, i32 -1931779049, !"_Z3barv", i32 45, i32 1}
!1 = !{i32 0, i32 53, i32 -1931779048, !"_Z3foov", i32 1, i32 0}

If you actually preprocessed the file first:
clang -E multiple_regions_per_line.cpp > m.cpp
Then compiled m.cpp instead you'd get 4:

!0 = !{i32 0, i32 66313, i32 56820824, !"_Z3foov", i32 26, i32 1}
!1 = !{i32 0, i32 66313, i32 56820824, !"_Z3foov", i32 16, i32 0}
!2 = !{i32 0, i32 66313, i32 56820824, !"_Z3barv", i32 37, i32 2}
!3 = !{i32 0, i32 66313, i32 56820824, !"_Z3barv", i32 41, i32 3}

But if we did that we might have a problem matching exactly the line numbers after preprocessing in host/device compiles.

ABataev added inline comments.Oct 14 2022, 12:11 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
3011–3012

Add an assert that no S in RegionCount before?

3035–3044

Maybe switch this 2 statements and check for S before and only if there is no S or no S in RegionCount, call getTargetRegionEntryInfoCount?
BTW, is possible that S has no associated record in RegionCount here?

jsjodin added a comment.EditedOct 14 2022, 12:40 PM

I think this makes sense, it just reminded me of the dense map nesting mess we have here. I never finished my patch to move this code and clean it up: https://github.com/jdoerfert/llvm-project/commit/cacae2849597a0b0ba9e3dd541467308bd6ef68d (@jsjodin, maybe you are interested in picking up my cleanup+move patch and integrating the count stuff of this patch as well?)

Yes, I'm actually migrating that code to OMPIRbuilder right now. We could integrate everything in one patch, or do them individually. I don't know which patch should go in first, it doesn't make too much of a difference imo. The only clang-specific types used in the entry and metadata generation is for error handling, which I created a callback function for in my patch. I haven't posted it yet, because of some lambda function capture issue, I could post it in case anyone wants to take a look at the code.

I will merge patch https://reviews.llvm.org/D135786 later today or tomorrow, or someone can do it for me if they feel like. So this patch will have to be modified.

mikerice added inline comments.Oct 14 2022, 3:19 PM
clang/lib/CodeGen/CGOpenMPRuntime.cpp
3035–3044

Yes, this is used both before and after 'S' is associated with a count so we need to call getTargetRegionEntryInfoCount when (!S or when not found). I was just trying not to have two calls in the code. Is this better:

llvm::Optional<unsigned> Count;
// If the region was seen before and a count was assigned, use it.
if (S) {
  auto CountIt = RegionCount.find(S);
  if (CountIt != RegionCount.end())
    Count = CountIt->second;
}
// Otherwise the count is the next available value.
if (!Count)
  Count =
      getTargetRegionEntryInfoCount(DeviceID, FileID, ParentName, LineNum);

I think this makes sense, it just reminded me of the dense map nesting mess we have here. I never finished my patch to move this code and clean it up: https://github.com/jdoerfert/llvm-project/commit/cacae2849597a0b0ba9e3dd541467308bd6ef68d (@jsjodin, maybe you are interested in picking up my cleanup+move patch and integrating the count stuff of this patch as well?)

Yes, I'm actually migrating that code to OMPIRbuilder right now. We could integrate everything in one patch, or do them individually. I don't know which patch should go in first, it doesn't make too much of a difference imo. The only clang-specific types used in the entry and metadata generation is for error handling, which I created a callback function for in my patch. I haven't posted it yet, because of some lambda function capture issue, I could post it in case anyone wants to take a look at the code.

I will merge patch https://reviews.llvm.org/D135786 later today or tomorrow, or someone can do it for me if they feel like. So this patch will have to be modified.

Any order is fine with me. We just need to coordinate. FWIW, please don't use 5 nested dense maps (see my patch).

I think this makes sense, it just reminded me of the dense map nesting mess we have here. I never finished my patch to move this code and clean it up: https://github.com/jdoerfert/llvm-project/commit/cacae2849597a0b0ba9e3dd541467308bd6ef68d (@jsjodin, maybe you are interested in picking up my cleanup+move patch and integrating the count stuff of this patch as well?)

Yes, I'm actually migrating that code to OMPIRbuilder right now. We could integrate everything in one patch, or do them individually. I don't know which patch should go in first, it doesn't make too much of a difference imo. The only clang-specific types used in the entry and metadata generation is for error handling, which I created a callback function for in my patch. I haven't posted it yet, because of some lambda function capture issue, I could post it in case anyone wants to take a look at the code.

I will merge patch https://reviews.llvm.org/D135786 later today or tomorrow, or someone can do it for me if they feel like. So this patch will have to be modified.

Any order is fine with me. We just need to coordinate. FWIW, please don't use 5 nested dense maps (see my patch).

I think it makes sense to do your cleanup first, then migrate to OMPIRbuilder, and do this feature improvement last. That way we have parity first then extend the functionality.

> I think it makes sense to do your cleanup first, then migrate to OMPIRbuilder, and do this feature improvement last. That way we have parity first then extend the functionality.

Makes sense to me too. @jsjodin are you going to add the cleanup or should I do it? After that I can fix this patch up.

> I think it makes sense to do your cleanup first, then migrate to OMPIRbuilder, and do this feature improvement last. That way we have parity first then extend the functionality.

Makes sense to me too. @jsjodin are you going to add the cleanup or should I do it? After that I can fix this patch up.

I am working on the cleanup. Doing some debugging, but the code is mostly there.

> I think it makes sense to do your cleanup first, then migrate to OMPIRbuilder, and do this feature improvement last. That way we have parity first then extend the functionality.

Makes sense to me too. @jsjodin are you going to add the cleanup or should I do it? After that I can fix this patch up.

I am working on the cleanup. Doing some debugging, but the code is mostly there.

I was being a bit too ambitious trying to combine the cleanup and the migration. I will split things up to simplify the changes.

> I think it makes sense to do your cleanup first, then migrate to OMPIRbuilder, and do this feature improvement last. That way we have parity first then extend the functionality.

Makes sense to me too. @jsjodin are you going to add the cleanup or should I do it? After that I can fix this patch up.

I am working on the cleanup. Doing some debugging, but the code is mostly there.

I was being a bit too ambitious trying to combine the cleanup and the migration. I will split things up to simplify the changes.

Here is the cleanup patch: https://reviews.llvm.org/D136601
Note that it changes the naming for variable because it was inconsistent with target regions "omp_offloading__..." vs "omp_offloading_..." so 1 underscore instead of 2 at the end of the prefix.

> I think it makes sense to do your cleanup first, then migrate to OMPIRbuilder, and do this feature improvement last. That way we have parity first then extend the functionality.

Makes sense to me too. @jsjodin are you going to add the cleanup or should I do it? After that I can fix this patch up.

I am working on the cleanup. Doing some debugging, but the code is mostly there.

I was being a bit too ambitious trying to combine the cleanup and the migration. I will split things up to simplify the changes.

Here is the cleanup patch: https://reviews.llvm.org/D136601
Note that it changes the naming for variable because it was inconsistent with target regions "omp_offloading__..." vs "omp_offloading_..." so 1 underscore instead of 2 at the end of the prefix.

The cleanup and migration patches are in now. We can proceed with this improvement. @mikerice are you going to work on this, or did you want me to?

The cleanup and migration patches are in now. We can proceed with this improvement. @mikerice are you going to work on this, or did you want me to?

Thanks @jsjodin. I've been trying to keep this patch up-to-date with the cleanup. I can update this review shortly.

mikerice updated this revision to Diff 472982.Nov 3 2022, 10:52 AM
mikerice edited the summary of this revision. (Show Details)
mikerice added a reviewer: jsjodin.

Rebase and update for new location of the OffloadEntriesInfoManager.

Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2022, 10:52 AM
jdoerfert added inline comments.Nov 3 2022, 11:23 PM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
4879 ↗(On Diff #472982)

Wasn't there a problem with . in symbols and ptx? Maybe it was a different character. I'd need to find the ... OK, found it: https://reviews.llvm.org/D80439

mikerice updated this revision to Diff 473257.Nov 4 2022, 9:04 AM
mikerice marked an inline comment as done.

Use underscore as count separator.

jdoerfert accepted this revision.Nov 4 2022, 10:23 AM
jdoerfert added a subscriber: jhuber6.

@jhuber6 Can you confirm this metadata is only used for the host - device compilation communication? If so, this looks good to me.

This revision is now accepted and ready to land.Nov 4 2022, 10:23 AM

@jhuber6 Can you confirm this metadata is only used for the host - device compilation communication? If so, this looks good to me.

As far as I know it's only relevant for making sure the host has the name of the device function so we can look it up at runtime.

Herald added a project: Restricted Project. · View Herald TranscriptNov 4 2022, 1:02 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript