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

[Andrew Gozillon via Phabricator via cfe-commits]

agozillon added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149162/new/

https://reviews.llvm.org/D149162