MaskRay added a comment. In D149162#4520497 <https://reviews.llvm.org/D149162#4520497>, @agozillon wrote:
> In D149162#4520395 <https://reviews.llvm.org/D149162#4520395>, @MaskRay wrote: > >> In D149162#4517500 <https://reviews.llvm.org/D149162#4517500>, @MaskRay >> wrote: >> >>> `registerTargetGlobalVariable` relies on the iteration order of StringMap, >>> which is not guaranteed. The bug is caught by D155789 >>> <https://reviews.llvm.org/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 <https://reviews.llvm.org/rG14c55e6e2fa1c342a1ef908445db3d31a3475485> 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149162/new/ https://reviews.llvm.org/D149162 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
