This is an archive of the discontinued LLVM Phabricator instance.

[Clang][OpenMP][IRBuilder] Move registerTargetGlobalVariable & getAddrOfDeclareTargetVar into the OMPIRBuilder
ClosedPublic

Authored by agozillon on Apr 25 2023, 8:08 AM.

Details

Summary

This change tries to move registerTargetglobalVariable and
getAddrOfDeclareTargetVar out of Clang's CGOpenMPRuntime
and into the OMPIRBuilder for shared use with MLIR's OpenMPDialect
and Flang (or other languages that may want to utilise it).

This primarily does this by trying to hoist the Clang specific
types into arguments or callback functions in the form of
lambdas, replacing it with LLVM equivelants and
utilising shared OMPIRBuilder enumerators for
the clauses, rather than Clang's own variation.

Diff Detail

Event Timeline

agozillon created this revision.Apr 25 2023, 8:08 AM
Herald added a project: Restricted Project. · View Herald Transcript
agozillon requested review of this revision.Apr 25 2023, 8:08 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript

I've tested this with the two following combinations, x86/AMDGPU and x86/NVPTX (newer plugin I think, not old, a little unsure on the runtime segment) and it passes the Clang test suites, if there is anything else I can do to make sure this is tested nicely and due diligence has been done please do mention it!

I admit the lengthy argument list is probably not the prettiest in the world, but the other option I could think of was an interface class of some kind that Clang+Flang could fill, or implement but that feels more like additional boilerplate than a fix.

jsjodin requested changes to this revision.Apr 26 2023, 7:09 AM
jsjodin added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
823

No llvm:: prefix needed.

882

Instead of passing in the various components I think it is better to pass in TargetRegionEntryInfo in case the inputs to create one changes. That way we avoid changing the parameters to this and the other functions.

883

Passing OpenMPISDevice shouldn't be necessary, the IsEmbedded flag in the Config should be used instead.

This revision now requires changes to proceed.Apr 26 2023, 7:09 AM
  • [Clang][OpenMP][IRBuilder] Tidy up function calls with helpful reviewer advice
  • [Clang][OpenMP][IRBuilder] Replace all getTargetEntryUniqueInfo with IRBuilder version
agozillon marked 3 inline comments as done.Apr 26 2023, 10:42 AM

Updated the patch with your feedback @jsjodin, thank you very much!

And I've updated all getTargetEntryUniqueInfo locations, which I neglected to do previously in this patch.

  • [Clang][OpenMP][IRBuilder] Run clang-format and tidy up files
agozillon updated this revision to Diff 517958.Apr 28 2023, 9:29 AM
  • [Clang][OpenMP][IRBuilder] Tidy up function calls with helpful reviewer advice
  • [Clang][OpenMP][IRBuilder] Replace all getTargetEntryUniqueInfo with IRBuilder version
  • [Clang][OpenMP][IRBuilder] Run clang-format and tidy up files
  • Run clang-format on offending file breaking build process

rebased and clang-formatted the *hopefully* final file clang-format is angry at.

agozillon updated this revision to Diff 518759.May 2 2023, 8:50 AM

Rebase on an updated main to see if it fixes bolt error (like it appears to on my local machine)

A small ping to ask for some reviewer attention on this patch if at all possible please! Thank you for your time as always, it is greatly appreciated.

jsjodin added inline comments.May 9 2023, 7:41 AM
llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h
871

Instead of passing in the Module, we should be able to use M in the OpenMPIRBuilder.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
5276

no llvm::prefix (look for more occurrences)

agozillon updated this revision to Diff 521528.May 11 2023, 6:55 PM
  • [Clang][OpenMP][IRBuilder] Tidy up function calls with helpful reviewer advice
  • [Clang][OpenMP][IRBuilder] Replace all getTargetEntryUniqueInfo with IRBuilder version
  • [Clang][OpenMP][IRBuilder] Run clang-format and tidy up files
  • Run clang-format on offending file breaking build process
  • Address reviewers comments
  • Add a new OpenMPIRBuilder that utilises registerTargetGlobalVariable (and by extension getAddrOfDeclareTargetVar) to generate host declare target data

In the last commit I believe I addressed the last review comments and I added an OMPIRBuilderTest testing some of the functionality of the registerTargetGlobalVariable and getAddrOfDeclareTargetVar functionality! Just to maintain the standard of making tests for things added to the OMPIRBuilder (the functions are also tested rather profusely by Clang and LLVM already).

@jdoerfert do you have any feedback?

clang/lib/CodeGen/CGOpenMPRuntime.cpp
1895

This logic is duplicated a few times, so a function would probably help.

  • [Clang][OpenMP][IRBuilder] Tidy up function calls with helpful reviewer advice
  • [Clang][OpenMP][IRBuilder] Replace all getTargetEntryUniqueInfo with IRBuilder version
  • [Clang][OpenMP][IRBuilder] Run clang-format and tidy up files
  • Run clang-format on offending file breaking build process
  • Address reviewers comments
  • Add a new OpenMPIRBuilder that utilises registerTargetGlobalVariable (and by extension getAddrOfDeclareTargetVar) to generate host declare target data
  • Add helper function to tidy things up a little as requested

rebased and added helper function as recommended, in recent update!

agozillon updated this revision to Diff 524279.May 22 2023, 6:40 AM

Rebase, squash patch history and clang-format to please the buildbot lords

small ping for a further review on this if possible! Thank you very much ahead of time.

jsjodin accepted this revision.Jun 6 2023, 7:16 AM

Looks good.

llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
5232

Nit: llvm::

This revision is now accepted and ready to land.Jun 6 2023, 7:16 AM

Thank you for the review @jsjodin I'll wait a couple of days to see if @jdoerfert (or anyone else) has any further input before I land the patch! I will apply the nit before landing the patch, so the final reference commit in Phabricator will reflect it.

jdoerfert accepted this revision.Jun 6 2023, 9:14 AM

I browsed it, looks fine.

agozillon added a comment.EditedJun 6 2023, 9:38 AM

Thank you both very much, I'll push this up tomorrow then when I'm better able to babysit the buildbots in-case of any CI issues as it's getting a little late in the EU (provided no one else has issues with the code of course between now and then)!

fmayer added a subscriber: fmayer.Jun 7 2023, 10:17 AM

This triggered an MSAN error: https://lab.llvm.org/buildbot/#/builders/237/builds/3127/steps/13/logs/stdio

           1: ==1184292==WARNING: MemorySanitizer: use-of-uninitialized-value 
check:6'0     X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
           2:  #0 0xaaaacb042944 in llvm::OpenMPIRBuilder::getTargetEntryUniqueInfo(llvm::StringRef, unsigned long, llvm::StringRef) /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:5205:10 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           3:  #1 0xaaaac3ee944c in getEntryInfoFromPresumedLoc(clang::CodeGen::CodeGenModule&, llvm::OpenMPIRBuilder&, clang::SourceLocation, llvm::StringRef) /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/CodeGen/CGOpenMPRuntime.cpp:1861:10 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           4:  #2 0xaaaac3f27708 in clang::CodeGen::CGOpenMPRuntime::emitTargetOutlinedFunctionHelper(clang::OMPExecutableDirective const&, llvm::StringRef, llvm::Function*&, llvm::Constant*&, bool, clang::CodeGen::RegionCodeGenTy const&) /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/CodeGen/CGOpenMPRuntime.cpp:6100:7 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           5:  #3 0xaaaac3f267a0 in clang::CodeGen::CGOpenMPRuntime::emitTargetOutlinedFunction(clang::OMPExecutableDirective const&, llvm::StringRef, llvm::Function*&, llvm::Constant*&, bool, clang::CodeGen::RegionCodeGenTy const&) /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/CodeGen/CGOpenMPRuntime.cpp:6040:3 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           6:  #4 0xaaaac4099080 in emitCommonOMPTargetDirective(clang::CodeGen::CodeGenFunction&, clang::OMPExecutableDirective const&, clang::CodeGen::RegionCodeGenTy const&) /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/CodeGen/CGStmtOpenMP.cpp:6625:26 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           .
           .
           .
          10:  #8 0xaaaac3beda74 in clang::CodeGen::CodeGenFunction::GenerateCode(clang::GlobalDecl, llvm::Function*, clang::CodeGen::CGFunctionInfo const&) /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/CodeGen/CodeGenFunction.cpp:1457:5 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          11:  #9 0xaaaac3a6a148 in clang::CodeGen::CodeGenModule::EmitGlobalFunctionDefinition(clang::GlobalDecl, llvm::GlobalValue*) /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:5446:26 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          12:  #10 0xaaaac3a55f14 in clang::CodeGen::CodeGenModule::EmitGlobalDefinition(clang::GlobalDecl, llvm::GlobalValue*) /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:3688:12 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          13:  #11 0xaaaac3a318bc in clang::CodeGen::CodeGenModule::EmitDeferred() /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:2840:5 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          14:  #12 0xaaaac3a2bf70 in clang::CodeGen::CodeGenModule::Release() /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/CodeGen/CodeGenModule.cpp:536:3 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          15:  #13 0xaaaac50aec24 in (anonymous namespace)::CodeGeneratorImpl::HandleTranslationUnit(clang::ASTContext&) /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/CodeGen/ModuleBuilder.cpp:287:18 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
check:6'1                ?                                                                                                                                                                                                        possible intended match
          16:  #14 0xaaaac50a751c in clang::BackendConsumer::HandleTranslationUnit(clang::ASTContext&) /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/CodeGen/CodeGenAction.cpp:311:14 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          17:  #15 0xaaaac8556f84 in clang::ParseAST(clang::Sema&, bool, bool) /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/Parse/ParseAST.cpp:176:13 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          18:  #16 0xaaaac4e8dcd0 in clang::FrontendAction::Execute() /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/Frontend/FrontendAction.cpp:1060:8 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          19:  #17 0xaaaac4d2994c in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:1049:33 
check:6'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          20:  #18 0xaaaac5096efc in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /b/sanitizer-aarch64-linux-bootstrap-msan/build/llvm-project/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:264:25

Thank you very much! I'm aware and currently trying to reproduce it locally and fix the issue.

fmayer added inline comments.Jun 7 2023, 10:35 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
5201–5204

This looks wrong. If this branch is entered, EC is true, so the assert never fails.

agozillon added inline comments.Jun 7 2023, 10:42 AM
llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
5201–5204

You are correct, thank you for the catch!

I have hopefully fixed the sanitizer issue (the incorrect assert, thank you again for the catch) with the following patch: https://github.com/llvm/llvm-project/commit/309023263dba3b02bc885101faa47d110f662f99 it was a slightly more involved change than I expected, I had to rework the getTargetEntryUniqueInfo function to use a callback function to support Clang's use of a fallback filename and line number (gathered from a PresumedLoc). So I apologies for the extended time to land the change.

fmayer added a comment.Jun 7 2023, 2:23 PM

I have hopefully fixed the sanitizer issue (the incorrect assert, thank you again for the catch) with the following patch: https://github.com/llvm/llvm-project/commit/309023263dba3b02bc885101faa47d110f662f99 it was a slightly more involved change than I expected, I had to rework the getTargetEntryUniqueInfo function to use a callback function to support Clang's use of a fallback filename and line number (gathered from a PresumedLoc). So I apologies for the extended time to land the change.

Thanks! I saw you use llvm_unreachable for error handling. Is that actually unreachable by construction of the program? Otherwise the compiler is free to assume this is never reached and will trigger undefined behaviour if it is.

I have hopefully fixed the sanitizer issue (the incorrect assert, thank you again for the catch) with the following patch: https://github.com/llvm/llvm-project/commit/309023263dba3b02bc885101faa47d110f662f99 it was a slightly more involved change than I expected, I had to rework the getTargetEntryUniqueInfo function to use a callback function to support Clang's use of a fallback filename and line number (gathered from a PresumedLoc). So I apologies for the extended time to land the change.

Thanks! I saw you use llvm_unreachable for error handling. Is that actually unreachable by construction of the program? Otherwise the compiler is free to assume this is never reached and will trigger undefined behaviour if it is.

I was actually unaware of that behavior, thank you very much for pointing it out!

The condition is reachable, but it's a condition that will result in incorrect kernel name generation in the resulting IR, so I imagine an assert is more apt then, I will push essentially the same line that's in this patch but inverted to be the right condition for it to trigger, i.e. !EC. Just running some local tests first to make sure all is fine. If you have another suggestion for the type of error construct to be used please do mention it however! I was considering report_fatal_error, but that appears to be for cases where compilation cannot continue.

MaskRay added a subscriber: MaskRay.EditedJul 20 2023, 12:29 AM

registerTargetGlobalVariable relies on the iteration order of StringMap, which is not guaranteed. The bug is caught by D155789

curl -L 'https://reviews.llvm.org/D155789?download=1' | patch -p1
cmake ... -DLLVM_ENABLE_REVERSE_ITERATION=on
ninja -C ... check-llvm-unit  #  `LLVM-Unit :: Frontend/./LLVMFrontendTests/OpenMPIRBuilderTest/registerTargetGlobalVariable` fails

registerTargetGlobalVariable relies on the iteration order of StringMap, which is not guaranteed. The bug is caught by D155789

curl -L 'https://reviews.llvm.org/D155789?download=1' | patch -p1
cmake ... -DLLVM_ENABLE_REVERSE_ITERATION=on
ninja -C ... check-llvm-unit  #  `LLVM-Unit :: Frontend/./LLVMFrontendTests/OpenMPIRBuilderTest/registerTargetGlobalVariable` fails

Looks like the issue might be introduced by 03f270c900e1f8563419fdd302683a9503e98722 in the iteration order of OffloadEntriesDeviceGlobalVarTy OffloadEntriesDeviceGlobalVar (a StringMap).
Unfortunately, changing it to MapVector<StringRef, OffloadEntryInfoDeviceGlobalVar> will break other tests like clang/test/OpenMP/declare_target_codegen.cpp, which suggests that there are other weird iteration order dependent behavior.
Hopefully someone familiar with OpenMP can investigate :)

registerTargetGlobalVariable relies on the iteration order of StringMap, which is not guaranteed. The bug is caught by D155789

curl -L 'https://reviews.llvm.org/D155789?download=1' | patch -p1
cmake ... -DLLVM_ENABLE_REVERSE_ITERATION=on
ninja -C ... check-llvm-unit  #  `LLVM-Unit :: Frontend/./LLVMFrontendTests/OpenMPIRBuilderTest/registerTargetGlobalVariable` fails

Looks like the issue might be introduced by 03f270c900e1f8563419fdd302683a9503e98722 in the iteration order of OffloadEntriesDeviceGlobalVarTy OffloadEntriesDeviceGlobalVar (a StringMap).
Unfortunately, changing it to MapVector<StringRef, OffloadEntryInfoDeviceGlobalVar> will break other tests like clang/test/OpenMP/declare_target_codegen.cpp, which suggests that there are other weird iteration order dependent behavior.
Hopefully someone familiar with OpenMP can investigate :)

I wasn't the original creator of this segment of code (03f270c900e1f8563419fdd302683a9503e98722) unfortunately, I just moved it into the OMPIRBuilder with some slight modifications, I can investigate it but I will unfortunately be on vacation from tomorrow onwards until the 9th of August, so if it is urgent it'd greatly be appreciated if someone else could have a look into it. Otherwise, I can pick it up when I get back.

If it is just the test added by this patch that is causing breakage and it affects no other OpenMP components currently, then you could perhaps deactivate this test for the time being until I can get time to look at it, if no one else has a moment to spare. It is perhaps because I am verifying the index value that's given to each piece of metadata/named global (e.g. test_data_int_1_decl_tgt_ref_ptr) and expecting they will remain the same in every case for this test, but with the introduction of LLVM_ENABLE_REVERSE_ITERATION that's perhaps no longer the case.

I am very sorry for the trouble.

registerTargetGlobalVariable relies on the iteration order of StringMap, which is not guaranteed. The bug is caught by D155789

curl -L 'https://reviews.llvm.org/D155789?download=1' | patch -p1
cmake ... -DLLVM_ENABLE_REVERSE_ITERATION=on
ninja -C ... check-llvm-unit  #  `LLVM-Unit :: Frontend/./LLVMFrontendTests/OpenMPIRBuilderTest/registerTargetGlobalVariable` fails

Looks like the issue might be introduced by 03f270c900e1f8563419fdd302683a9503e98722 in the iteration order of OffloadEntriesDeviceGlobalVarTy OffloadEntriesDeviceGlobalVar (a StringMap).
Unfortunately, changing it to MapVector<StringRef, OffloadEntryInfoDeviceGlobalVar> will break other tests like clang/test/OpenMP/declare_target_codegen.cpp, which suggests that there are other weird iteration order dependent behavior.
Hopefully someone familiar with OpenMP can investigate :)

I wasn't the original creator of this segment of code (03f270c900e1f8563419fdd302683a9503e98722) unfortunately, I just moved it into the OMPIRBuilder with some slight modifications, I can investigate it but I will unfortunately be on vacation from tomorrow onwards until the 9th of August, so if it is urgent it'd greatly be appreciated if someone else could have a look into it. Otherwise, I can pick it up when I get back.

If it is just the test added by this patch that is causing breakage and it affects no other OpenMP components currently, then you could perhaps deactivate this test for the time being until I can get time to look at it, if no one else has a moment to spare. It is perhaps because I am verifying the index value that's given to each piece of metadata/named global (e.g. test_data_int_1_decl_tgt_ref_ptr) and expecting they will remain the same in every case for this test, but with the introduction of LLVM_ENABLE_REVERSE_ITERATION that's perhaps no longer the case.

I am very sorry for the trouble.

Thank you for offering help! No hurry. For LLVM_ENABLE_REVERSE_ITERATION, my commit 14c55e6e2fa1c342a1ef908445db3d31a3475485 has worked for me.

The underlying issue that !omp_offload.info has an operand order that is dependent on StringMap can be fixed later :) FWIW one of my attempts was:

--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -5982,8 +5982,12 @@ void OffloadEntriesInfoManager::registerDeviceGlobalVarEntryInfo(
 void OffloadEntriesInfoManager::actOnDeviceGlobalVarEntriesInfo(
     const OffloadDeviceGlobalVarEntryInfoActTy &Action) {
   // Scan all target region entries and perform the provided action.
+  SmallVector<std::pair<StringRef, OffloadEntryInfoDeviceGlobalVar>, 0> Vec;
   for (const auto &E : OffloadEntriesDeviceGlobalVar)
-    Action(E.getKey(), E.getValue());
+    Vec.emplace_back(E.getKey(), E.getValue());
+  llvm::sort(Vec, less_first());
+  for (const auto &E : Vec)
+    Action(E.first, E.second);
 }

 void CanonicalLoopInfo::collectControlBlocks(

and it would break clang/test/OpenMP/declare_target_codegen.cpp, so I didn't investigate further.

registerTargetGlobalVariable relies on the iteration order of StringMap, which is not guaranteed. The bug is caught by D155789

curl -L 'https://reviews.llvm.org/D155789?download=1' | patch -p1
cmake ... -DLLVM_ENABLE_REVERSE_ITERATION=on
ninja -C ... check-llvm-unit  #  `LLVM-Unit :: Frontend/./LLVMFrontendTests/OpenMPIRBuilderTest/registerTargetGlobalVariable` fails

Looks like the issue might be introduced by 03f270c900e1f8563419fdd302683a9503e98722 in the iteration order of OffloadEntriesDeviceGlobalVarTy OffloadEntriesDeviceGlobalVar (a StringMap).
Unfortunately, changing it to MapVector<StringRef, OffloadEntryInfoDeviceGlobalVar> will break other tests like clang/test/OpenMP/declare_target_codegen.cpp, which suggests that there are other weird iteration order dependent behavior.
Hopefully someone familiar with OpenMP can investigate :)

I wasn't the original creator of this segment of code (03f270c900e1f8563419fdd302683a9503e98722) unfortunately, I just moved it into the OMPIRBuilder with some slight modifications, I can investigate it but I will unfortunately be on vacation from tomorrow onwards until the 9th of August, so if it is urgent it'd greatly be appreciated if someone else could have a look into it. Otherwise, I can pick it up when I get back.

If it is just the test added by this patch that is causing breakage and it affects no other OpenMP components currently, then you could perhaps deactivate this test for the time being until I can get time to look at it, if no one else has a moment to spare. It is perhaps because I am verifying the index value that's given to each piece of metadata/named global (e.g. test_data_int_1_decl_tgt_ref_ptr) and expecting they will remain the same in every case for this test, but with the introduction of LLVM_ENABLE_REVERSE_ITERATION that's perhaps no longer the case.

I am very sorry for the trouble.

Thank you for offering help! No hurry. For LLVM_ENABLE_REVERSE_ITERATION, my commit 14c55e6e2fa1c342a1ef908445db3d31a3475485 has worked for me.

The underlying issue that !omp_offload.info has an operand order that is dependent on StringMap can be fixed later :) FWIW one of my attempts was:

--- a/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
+++ b/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp
@@ -5982,8 +5982,12 @@ void OffloadEntriesInfoManager::registerDeviceGlobalVarEntryInfo(
 void OffloadEntriesInfoManager::actOnDeviceGlobalVarEntriesInfo(
     const OffloadDeviceGlobalVarEntryInfoActTy &Action) {
   // Scan all target region entries and perform the provided action.
+  SmallVector<std::pair<StringRef, OffloadEntryInfoDeviceGlobalVar>, 0> Vec;
   for (const auto &E : OffloadEntriesDeviceGlobalVar)
-    Action(E.getKey(), E.getValue());
+    Vec.emplace_back(E.getKey(), E.getValue());
+  llvm::sort(Vec, less_first());
+  for (const auto &E : Vec)
+    Action(E.first, E.second);
 }

 void CanonicalLoopInfo::collectControlBlocks(

and it would break clang/test/OpenMP/declare_target_codegen.cpp, so I didn't investigate further.

Thank you very much for looking into it and fixing it for the time being, I'm sorry for the bother! I'll add it to my TODO list for when I get back to have a deeper look into it, if someone else doesn't get to it first.