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

agozillon added a comment. In D149162#4520747 , @MaskRay wrote: > In D149162#4520497 , @agozillon > wrote: > >> In D149162#4520395 , @MaskRay >> wrote: >> >>> In

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

MaskRay added a comment. In D149162#4520497 , @agozillon wrote: > In D149162#4520395 , @MaskRay wrote: > >> In D149162#4517500 , @MaskRay >> wrote: >> >>>

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

agozillon added a comment. In D149162#4520395 , @MaskRay wrote: > In D149162#4517500 , @MaskRay wrote: > >> `registerTargetGlobalVariable` relies on the iteration order of StringMap, >> which is not guaranteed.

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

MaskRay added a comment. In D149162#4517500 , @MaskRay wrote: > `registerTargetGlobalVariable` relies on the iteration order of StringMap, > which is not guaranteed. The bug is caught by D155789 > > > curl -L

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

MaskRay added a comment. `registerTargetGlobalVariable` has a iteration order issue using StringMap, caught by D155789 curl -L 'https://reviews.llvm.org/D155789?download=1' | patch -p1 cmake ... -DLLVM_ENABLE_REVERSE_ITERATION=on ninja -C ...

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

agozillon added a comment. In D149162#4404379 , @fmayer wrote: > In D149162#4404271 , @agozillon > wrote: > >> I have hopefully fixed the sanitizer issue (the incorrect assert, thank you >> again for the catch)

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

fmayer added a comment. In D149162#4404271 , @agozillon wrote: > I have hopefully fixed the sanitizer issue (the incorrect assert, thank you > again for the catch) with the following patch: >

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

agozillon added a comment. 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

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

agozillon added inline comments. Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:5201-5204 + if (auto EC = sys::fs::getUniqueID(FileName, ID)) { +assert(EC && + "Unable to get unique ID for file, during getTargetEntryUniqueInfo"); + }

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

fmayer added inline comments. Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:5201-5204 + if (auto EC = sys::fs::getUniqueID(FileName, ID)) { +assert(EC && + "Unable to get unique ID for file, during getTargetEntryUniqueInfo"); + } This

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

agozillon added a comment. Thank you very much! I'm aware and currently trying to reproduce it locally and fix the issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149162/new/ https://reviews.llvm.org/D149162

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

fmayer added a comment. 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~~~

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGcda46cc4f921: [Clang][OpenMP][IRBuilder] Move registerTargetGlobalVariable … (authored by agozillon). Changed prior to commit: https://reviews.llvm.org/D149162?vs=524279=529258#toc Repository: rG

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

agozillon added a comment. Thank you both very much, I'll push this up tomorrow then when I'm better able to babysit the buildbot 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)! Repository: rG

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

jdoerfert accepted this revision. jdoerfert added a comment. I browsed it, looks fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149162/new/ https://reviews.llvm.org/D149162 ___ cfe-commits mailing

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

agozillon added a comment. 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.

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

jsjodin accepted this revision. jsjodin added a comment. This revision is now accepted and ready to land. Looks good. Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:5230 + if (!IsExternallyVisible) +OS << llvm::format("_%x", EntryInfo.FileID); + OS <<

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

agozillon added a comment. small ping for a further review on this if possible! Thank you very much ahead of time. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149162/new/ https://reviews.llvm.org/D149162

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

agozillon updated this revision to Diff 524279. agozillon added a comment. Rebase, squash patch history and clang-format to please the buildbot lords Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149162/new/ https://reviews.llvm.org/D149162

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

agozillon added a comment. rebased and added helper function as recommended, in recent update! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149162/new/ https://reviews.llvm.org/D149162 ___ cfe-commits

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

agozillon updated this revision to Diff 522710. agozillon added a comment. - [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

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

jsjodin added a comment. @jdoerfert do you have any feedback? Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:1882 + auto EntryInfo = OMPBuilder.getTargetEntryUniqueInfo( + PLoc.getFilename(), PLoc.getLine(), VD->getName()); SmallString<128> Buffer, Out;

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

agozillon added a comment. 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

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

agozillon updated this revision to Diff 521528. agozillon added a comment. - [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

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

jsjodin added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:865 + TargetRegionEntryInfo EntryInfo, StringRef MangledName, + Module *LlvmModule, std::vector , + bool OpenMPSIMD, std::vector TargetTriple, Instead of

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

agozillon added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149162/new/

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

agozillon updated this revision to Diff 518759. agozillon added a comment. Rebase on an updated main to see if it fixes bolt error (like it appears to on my local machine) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149162/new/

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

agozillon added a comment. rebased and clang-formatted the *hopefully* final file clang-format is angry at. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149162/new/ https://reviews.llvm.org/D149162 ___

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

agozillon updated this revision to Diff 517958. agozillon added a comment. - [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

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

agozillon updated this revision to Diff 517264. agozillon added a comment. - [Clang][OpenMP][IRBuilder] Run clang-format and tidy up files Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149162/new/ https://reviews.llvm.org/D149162 Files:

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

agozillon marked 3 inline comments as done. agozillon added a comment. 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. Repository: rG LLVM Github Monorepo CHANGES

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

agozillon updated this revision to Diff 517235. agozillon added a comment. - [Clang][OpenMP][IRBuilder] Tidy up function calls with helpful reviewer advice - [Clang][OpenMP][IRBuilder] Replace all getTargetEntryUniqueInfo with IRBuilder version Repository: rG LLVM Github Monorepo CHANGES

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

jsjodin requested changes to this revision. jsjodin added inline comments. This revision now requires changes to proceed. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:817 + /// LLVMModule. + llvm::Constant *getAddrOfDeclareTargetVar( +

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

agozillon added a comment. 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

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

agozillon created this revision. Herald added subscribers: sunshaoce, bzcheeseman, rriddle, rogfer01, guansong, hiraditya, yaxunl. Herald added a reviewer: sscalpone. Herald added a project: All. agozillon requested review of this revision. Herald added a reviewer: jdoerfert. Herald added