This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP][OMPIRBuilder] OpenMPIRBuilder support for requires directive
ClosedPublic

Authored by skatrak on Mar 30 2023, 4:32 AM.

Details

Summary

This patch updates the OpenMPIRBuilderConfig structure to hold all
available 'requires' clauses, and it replicates part of the code
generation for the 'requires' registration function from clang in the
OMPIRBuilder, to be used with flang.

Porting the rest of features of the clang implementation to the IRBuilder
and sharing it by clang and flang remains for a future patch, due to the
complexity of the logic selecting the attributes of the generated
registration function.

Diff Detail

Event Timeline

skatrak created this revision.Mar 30 2023, 4:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 30 2023, 4:32 AM
skatrak requested review of this revision.Mar 30 2023, 4:32 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
skatrak updated this revision to Diff 512805.Apr 12 2023, 6:17 AM

Add unit test.

skatrak updated this revision to Diff 515684.Apr 21 2023, 5:02 AM

Avoid creating registration function for the device.

skatrak edited the summary of this revision. (Show Details)Apr 21 2023, 6:58 AM
skatrak edited the summary of this revision. (Show Details)Apr 21 2023, 6:59 AM
skatrak edited the summary of this revision. (Show Details)

I have been able to track down the failed lld.wasm::stub_library.s unit test to be due to the buildbot picking up line endings for the lld/test/wasm/Inputs/libstub.so to be Windows ones ("\r\n"), so the condition if (mbref.getBuffer().starts_with("#STUB\n")) in lld/wasm/Driver.cpp:285 evaluates to false and the file is not processed. I'm not sure why this problem is being specifically picked up by the buildbot for this patch and whether it's already fixed upstream.

skatrak updated this revision to Diff 518677.May 2 2023, 3:38 AM
skatrak edited the summary of this revision. (Show Details)

Rebase

Herald added a project: Restricted Project. · View Herald Transcript
jsjodin added inline comments.May 16 2023, 7:49 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
5291

Perhaps this function should be part of the subsequent patches, since we're only moving and using the configuration bits right now.

skatrak added inline comments.May 31 2023, 10:09 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
5291

Thank you for the comment. My thinking was to keep all OMPIRBuilder changes self-contained in a single patch, which also allowed me to create a unit test for it independent from other changes and make it easier to review. I could merge this function and the new unit test into the dependent patch D147219, but maybe that one is large enough as it is.

I suppose your concern is having this function make its way into the trunk without any uses. Would keeping these changes on this patch but only landing them whenever both patches are accepted be a reasonable solution?

skatrak updated this revision to Diff 527380.Jun 1 2023, 5:46 AM

Update with latest main branch.

jsjodin accepted this revision.Jun 13 2023, 8:29 AM

I think this is fine. The dependent diff has already been approved.

This revision is now accepted and ready to land.Jun 13 2023, 8:29 AM
skatrak updated this revision to Diff 551088.Aug 17 2023, 4:42 AM

Update patch.

skatrak updated this revision to Diff 551150.Aug 17 2023, 8:35 AM

Fix formatting.

skatrak updated this revision to Diff 551454.Aug 18 2023, 3:25 AM

Rebase patch to fix unrelated build error.

skatrak updated this revision to Diff 552427.Aug 22 2023, 10:36 AM

Update patch.