[PATCH] D149162: [Clang][OpenMP][IRBuilder] Move registerTargetGlobalVariable & getAddrOfDeclareTargetVar into the OMPIRBuilder

Andrew Gozillon via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 20 16:16:38 PDT 2023

agozillon added a comment.

In D149162#4520747 <https://reviews.llvm.org/D149162#4520747>, @MaskRay wrote:

> 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
>>>>   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.

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.

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list