[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

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

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

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

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

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

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

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] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

This revision was automatically updated to reflect the committed changes. Closed by commit rG48c3ae5cc3d4: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and… (authored by agozillon). Changed prior to commit: https://reviews.llvm.org/D148370?vs=519050=522671#toc Repository: rG LLVM

[PATCH] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon added a comment. I am going to upstream this within the next hour, unless anyone has some final comments they wish to make Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148370/new/ https://reviews.llvm.org/D148370

[PATCH] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon added a comment. Thank you very much @kiranchandramohan! More than happy to wait a day incase anyone else wishes to do a final review. I'll add the Nits where possible when I commit the patch upstream and it should hopefully update the review with the alterations! Repository: rG

[PATCH] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon added a comment. If it would be possible to get some poor reviewers precious time on this I would greatly appreciate it! Thank you very much ahead of time. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148370/new/

[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] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon added a comment. Hey @jdoerfert sorry to bother you, would it be possible to have this signed off on if there is no further issues with the current patch? Thank you for your time! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon added a comment. Updated to fix the style issues that were present and pointed out by @jdoerfert thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148370/new/ https://reviews.llvm.org/D148370

[PATCH] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon updated this revision to Diff 519050. agozillon marked an inline comment as done. agozillon added a comment. - Remove unneccessary OMPIRBuilder scope as well Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148370/new/

[PATCH] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon updated this revision to Diff 519048. agozillon added a comment. - Move hostIRFilePath initialize invocation to ModuleTranslation.cpp to respect TargetOp patch - Apply reviewer feedback - Tidy up missed style guidelines i sillily forgot about Repository: rG LLVM Github Monorepo

[PATCH] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon added a comment. Thank you very much for the quick response time on the review and the review @jdoerfert! I believe I have applied all of your current feedback in the last update. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon updated this revision to Diff 518875. agozillon marked 4 inline comments as done. agozillon added a comment. - Move hostIRFilePath initialize invocation to ModuleTranslation.cpp to respect TargetOp patch - Apply reviewer feedback Repository: rG LLVM Github Monorepo CHANGES SINCE

[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] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon added a comment. Small ping to ask for some reviewer attention on this patch if at all possible! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148370/new/ https://reviews.llvm.org/D148370 ___

[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] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon added a comment. rebase to see if it clears up the buildbot issue (currently the failing test appears to be unrelated and passes on my machine after a rebase) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148370/new/

[PATCH] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon updated this revision to Diff 517968. agozillon added a comment. - Move hostIRFilePath initialize invocation to ModuleTranslation.cpp to respect TargetOp patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148370/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] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon updated this revision to Diff 517682. agozillon added a comment. - Move hostIRFilePath initialize invocation to ModuleTranslation.cpp to respect TargetOp patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148370/new/

[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

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

[PATCH] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon added a comment. The patch this is dependent on has now been accepted and landed, so this should be ready for review now if anyone is available! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148370/new/ https://reviews.llvm.org/D148370

[PATCH] D148038: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR module attribute to proliferate to OpenMP IR lowering

agozillon added a comment. In D148038#4292262 , @awarzynski wrote: > LGTM, thanks for addressing my comments! Thank you for your time and the great review comments as always! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D148038: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR module attribute to proliferate to OpenMP IR lowering

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. agozillon marked an inline comment as done. Closed by commit rG2e634367be6a: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR… (authored

[PATCH] D148038: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR module attribute to proliferate to OpenMP IR lowering

agozillon marked an inline comment as done. agozillon added inline comments. Comment at: flang/lib/Frontend/CompilerInvocation.cpp:730 +res.getLangOpts().OMPHostIRFile = arg->getValue(); +if (!llvm::sys::fs::exists(res.getLangOpts().OMPHostIRFile)) +

[PATCH] D148038: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR module attribute to proliferate to OpenMP IR lowering

agozillon updated this revision to Diff 516374. agozillon added a comment. - Resolve some of the reviewer comments - [Flang][OpenMP][Driver][MLIR] Add diagnostic error test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148038/new/

[PATCH] D148038: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR module attribute to proliferate to OpenMP IR lowering

agozillon marked 4 inline comments as done. agozillon added a comment. Addressed 4/5 of your comments in the latest patch @awarzynski, I've replied to the final one to give a little more information to add to the final decision you make! Comment at:

[PATCH] D148038: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR module attribute to proliferate to OpenMP IR lowering

agozillon updated this revision to Diff 515305. agozillon added a comment. - Resolve some of the reviewer comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148038/new/ https://reviews.llvm.org/D148038 Files:

[PATCH] D145264: [OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr

agozillon abandoned this revision. agozillon added a comment. Herald added a subscriber: bviyer. Closing this patch now that each individual component is upstreamed via seperate phabricator patches! Thank you to all the reviewers for your time and help. Repository: rG LLVM Github Monorepo

[PATCH] D148038: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR module attribute to proliferate to OpenMP IR lowering

agozillon added a comment. Just a small ping to keep this patch on peoples radar and to try and get some reviewer attention (I am aware you're all quite busy, so thank you very much for any time you can spare) for more review feedback to push this patch further along the pipeline!

[PATCH] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon added inline comments. Comment at: mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h:174 +if (offloadMod.getIsDevice()) { + hostIRFilePath = offloadMod.getHostIRFilePath(); +} This will break the patch buildbot unfortunately

[PATCH] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon added a comment. A little unsure how to test this change on its own for Flang-new + OpenMP, as there currently isn't anything that utilises it upstream at the moment. It's part of the declare target work that I'm beginning to upstream, so eventually it'll be tested on the Flang side

[PATCH] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

agozillon created this revision. Herald added subscribers: bviyer, sunshaoce, Moerafaat, zero9178, bzcheeseman, awarzynski, sdasgup3, wenzhicui, wrengr, cota, teijeong, rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, mgester, arpith-jacob, antiagainst,

[PATCH] D148038: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR module attribute to proliferate to OpenMP IR lowering

agozillon updated this revision to Diff 512554. agozillon added a comment. Adding missing newline Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D148038/new/ https://reviews.llvm.org/D148038 Files: clang/include/clang/Driver/Options.td

[PATCH] D148038: [Flang][OpenMP][Driver][MLIR] Port fopenmp-host-ir-file-path flag and add MLIR module attribute to proliferate to OpenMP IR lowering

agozillon created this revision. Herald added subscribers: bviyer, sunshaoce, Moerafaat, zero9178, bzcheeseman, sdasgup3, wenzhicui, wrengr, cota, teijeong, rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, mgester, arpith-jacob, antiagainst, shauheen,

[PATCH] D147324: [OpenMP][MLIR][Flang][bbc][Driver] Add OpenMP RTL Flags to Flang and generate omp.FlagsAttr from them

This revision was automatically updated to reflect the committed changes. Closed by commit rG53152f12a47b: [OpenMP][MLIR][Flang][bbc][Driver] Add OpenMP RTL Flags to Flang and generate… (authored by agozillon). Changed prior to commit: https://reviews.llvm.org/D147324?vs=510080=511154#toc

[PATCH] D147324: [OpenMP][MLIR][Flang][bbc][Driver] Add OpenMP RTL Flags to Flang and generate omp.FlagsAttr from them

agozillon added a comment. In D147324#4245932 , @kiranchandramohan wrote: > LG. This portion of the patch was accepted in > https://reviews.llvm.org/D145264. > > See inline comment about location of LLVM Dialect tests. You may either > remove the LLVM

[PATCH] D147324: [OpenMP][MLIR][Flang][bbc][Driver] Add OpenMP RTL Flags to Flang and generate omp.FlagsAttr from them

agozillon added a comment. Hi @awarzynski would it please be possible to get this reviewed when you have a spare moment or few, it's essentially the segment of the modifications from this patch which you accepted: https://reviews.llvm.org/D145264 the lowering has been separated into a

[PATCH] D145264: [OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr

agozillon added a comment. Fragmented this patch into https://reviews.llvm.org/D147344 (lowering) and https://reviews.llvm.org/D147324 (driver/tool changes and application of attribute). I will keep this patch open until the others are closed to give a big picture for easier reference.

[PATCH] D147324: [OpenMP][MLIR][Flang][bbc][Driver] Add OpenMP RTL Flags to Flang and generate omp.FlagsAttr from them

agozillon updated this revision to Diff 510080. agozillon added a comment. - Add FIR -> LLVM Dialect test here, more fitting than the other patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147324/new/ https://reviews.llvm.org/D147324 Files:

[PATCH] D147324: [OpenMP][MLIR][Flang][bbc][Driver] Add OpenMP RTL Flags to Flang and generate omp.FlagsAttr from them

agozillon added a comment. The flag addition and FlagsAttr generation component of: https://reviews.llvm.org/D145264 the LLVM-IR generation will come in a subsequent phabricator patch once I have created some additional tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D147324: [OpenMP][MLIR][Flang][bbc][Driver] Add OpenMP RTL Flags to Flang and generate omp.FlagsAttr from them

agozillon created this revision. Herald added subscribers: sunshaoce, bzcheeseman, rriddle, guansong, yaxunl. Herald added a reviewer: sscalpone. Herald added a reviewer: awarzynski. Herald added a reviewer: kiranchandramohan. Herald added projects: Flang, All. agozillon requested review of this

[PATCH] D145264: [OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr

agozillon added a comment. In D145264#4236173 , @kiranchandramohan wrote: > Please split this patch into three: > > 1. Code changes and testing for the driver and the FIR+OpenMP dialect > generated. > 2. Code changes and test for FIR+OpenMP to

[PATCH] D145264: [OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr

agozillon added inline comments. Comment at: clang/include/clang/Driver/Options.td:2692 def fno_openmp_assume_teams_oversubscription : Flag<["-"], "fno-openmp-assume-teams-oversubscription">, - Group, Flags<[CC1Option, NoArgumentUnused, HelpHidden]>; + Group,

[PATCH] D145264: [OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr

agozillon updated this revision to Diff 509837. agozillon added a comment. Squash commits, format and apply requested changes Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145264/new/ https://reviews.llvm.org/D145264 Files:

[PATCH] D145264: [OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr

agozillon added inline comments. Comment at: flang/tools/bbc/CMakeLists.txt:29 FortranLower +flangFrontendTool ) agozillon wrote: > awarzynski wrote: > > This a frontend driver library and so far `bbc` and `flang-new -fc1` have > > been entirely separate.

[PATCH] D145264: [OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr

agozillon added inline comments. Comment at: clang/include/clang/Driver/Options.td:2692 def fno_openmp_assume_teams_oversubscription : Flag<["-"], "fno-openmp-assume-teams-oversubscription">, - Group, Flags<[CC1Option, NoArgumentUnused, HelpHidden]>; + Group,

[PATCH] D145264: [OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr

agozillon added a comment. Unfortunately I do not believe an mlir-translate test that tests if the OffloadModuleInterface is accessible when directly utilizing mlir-translate is possible for this patch... I forgot I removed the is device check as it is already done at the initial creation of

[PATCH] D145264: [OpenMP][MLIR] Lower and apply RTLModuleFlagsAttr

agozillon added a comment. Recent update(s) moved flag/driver testing of the flags from Clang to Flang's omp-frontend-forwarding.f90 aligning it with previous tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145264/new/

[PATCH] D145264: [OpenMP][MLIR] Lower and apply RTLModuleFlagsAttr

agozillon updated this revision to Diff 509628. agozillon added a comment. - [Flang][Driver][Test] Move flang-omp.f90 tests into omp-frontend-forwarding.f90 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145264/new/

[PATCH] D145264: [OpenMP][MLIR] Lower and apply RTLModuleFlagsAttr

agozillon updated this revision to Diff 509626. agozillon added a comment. - [Flang][Driver][Test] Move flang-omp.f90 tests into omp-frontend-forwarding.f90 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145264/new/

[PATCH] D145264: [OpenMP][MLIR] Lower and apply RTLModuleFlagsAttr

agozillon added a comment. All the mechanisms for this patch to now function are upstream, so I believe it is ready for a review if at all possible @kiranchandramohan @awarzynski and other reviewers that this patch may be of interest to. Although if we wish for it to be split into two separate

[PATCH] D145264: [OpenMP][MLIR] Lower and apply RTLModuleFlagsAttr

agozillon updated this revision to Diff 509487. agozillon edited the summary of this revision. agozillon added a comment. Herald added subscribers: jplehr, sunshaoce. - [Flang][MLIR][OpenMP] Fix rebase inconsistencies - [Flang][bbc][Tools] Add OpenMP RTL flags to bbc and adjust

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon added a comment. No problem at all, I broke it, happy to fix it! Submitting the fix in the next 30 minutes~ after a rebase as the build bots seem to have passed happily, it took a lot longer than I expected for the build-bot to roundtable to my patch, my apologies, teaches me to use

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon added a comment. In D145815#4230780 , @jeanPerier wrote: > @agozillon, in the test added here (omp-frontend-forwarding.f90), I am seeing > failures in some patches windows pre-merge checks that I think are not > related to the patches. >

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

This revision was automatically updated to reflect the committed changes. Closed by commit rG0cd31a7d3087: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to… (authored by agozillon). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon added a comment. May I please request a final acceptance from both @jhuber6 and @awarzynski before I commit this upstream! If you have no further comments to add or requests of course. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon added a comment. Recent commit added some more detail to the comment on the fembed-offload-object argument as requested by @awarzynski Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145815/new/ https://reviews.llvm.org/D145815

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon updated this revision to Diff 505957. agozillon edited the summary of this revision. agozillon added a comment. - [Flang][Driver] Expand further on the embed-offload-object comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145815/new/

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon added inline comments. Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128 + if (IsHostOffloadingAction) { +for (size_t i = 1; i < Inputs.size(); ++i) { + if (Inputs[i].getType() != types::TY_Nothing) awarzynski wrote: > jhuber6 wrote: > >

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon added inline comments. Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128 + if (IsHostOffloadingAction) { +for (size_t i = 1; i < Inputs.size(); ++i) { + if (Inputs[i].getType() != types::TY_Nothing) jhuber6 wrote: > agozillon wrote: > >

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon added a comment. Deleted the clang test that was forwarding to Flang and merged with the omp-frontend-forwarding.f90 test where relevant. Second push was because I forgot to add a missing newline, which I seem to do frequently... Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon updated this revision to Diff 505844. agozillon added a comment. - Readd missing newline Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145815/new/ https://reviews.llvm.org/D145815 Files: clang/lib/Driver/ToolChains/Flang.cpp

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon added a comment. In D145815#4199848 , @tschuett wrote: > Flang will also support OpenACC for offload. It is very similar to OpenMP. You are correct, thank you for reminding me! No idea how I forgot it, my apologies. Repository: rG LLVM

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon updated this revision to Diff 505842. agozillon added a comment. - [Flang][Driver] Try to fix failing omp-frontend-forwarding.f90 test - [Flang][Driver] Simplify omp-frontend-forwarding.f90 test - [Flang][Driver][Test] Delete flang-omp test from Clang and merge tests into

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon added inline comments. Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128 + if (IsHostOffloadingAction) { +for (size_t i = 1; i < Inputs.size(); ++i) { + if (Inputs[i].getType() != types::TY_Nothing) awarzynski wrote: > agozillon wrote: >

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon added inline comments. Comment at: clang/lib/Driver/ToolChains/Flang.cpp:128 + if (IsHostOffloadingAction) { +for (size_t i = 1; i < Inputs.size(); ++i) { + if (Inputs[i].getType() != types::TY_Nothing) awarzynski wrote: > What's the magic

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon added a comment. Recent update was to simplify the omp-frontend-forwarding.f90 test Comment at: flang/test/Driver/omp-frontend-forwarding.f90:1 +! REQUIRES: amdgpu-registered-target + awarzynski wrote: > Given that you use `-###`, I think that this

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon updated this revision to Diff 504829. agozillon added a comment. - [Flang][Driver] Simplify omp-frontend-forwarding.f90 test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145815/new/ https://reviews.llvm.org/D145815 Files:

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon added inline comments. Comment at: flang/test/Driver/omp-frontend-forwarding.f90:15 +! CHECK: "{{[^"]*}}flang-new" "-fc1" "-triple" "amdgcn-amd-amdhsa" {{.*}} "-fopenmp" {{.*}} "-fopenmp-is-device" {{.*}}.f90" +! CHECK: "{{[^"]*}}clang-offload-packager" {{.*}}

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon updated this revision to Diff 504592. agozillon added a comment. - [Flang][Driver] Try to fix failing omp-frontend-forwarding.f90 test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145815/new/ https://reviews.llvm.org/D145815 Files:

[PATCH] D145815: [Flang][Driver] Add support for fopenmp-is-device and fembed-offload-object to Flang ToolChain

agozillon created this revision. Herald added subscribers: sunshaoce, guansong, yaxunl. Herald added a reviewer: sscalpone. Herald added a reviewer: awarzynski. Herald added projects: Flang, All. agozillon requested review of this revision. Herald added subscribers: cfe-commits, sstefan1,

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe002a38b20e3: [Flang][OpenMP][MLIR][Driver][bbc] Add -fopenmp-is-device flag to Flang -fc1 … (authored by agozillon). Repository: rG LLVM Github

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

agozillon added a comment. Cleaned up the test, thank you both for teaching me more about the compiler and test infrastructure! If you're happy with the test changes @kiranchandramohan I'll commit the changes upstream to close out the review? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

agozillon updated this revision to Diff 503070. agozillon added a comment. - [Flang][Driver] Tidy up omp-is-device.f90 test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144864/new/ https://reviews.llvm.org/D144864 Files:

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

agozillon added a comment. In D144864#4175257 , @kiranchandramohan wrote: > LG. See one minor comment in the tests. > > I would prefer having an Interface for Target Modules if that could be made > to work. I guess this can be taken up separately after

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

agozillon added a comment. The recent update removed the added offload related code from Flang.h/.cpp and removed the related test. Would it be possible to have an extra sign-off as requested by @awarzynski (or more review points to correct/discuss if we aren't happy with the state/direction

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

agozillon updated this revision to Diff 503008. agozillon added a comment. - [Flang][ToolChain][OpenMP][Driver] Reduce changes to only add -fc1 support Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144864/new/ https://reviews.llvm.org/D144864

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

agozillon added inline comments. Comment at: clang/test/Driver/flang/flang-omp.f90:1 +! Check that flang -fc1 is invoked when in --driver-mode=flang +! and the relevant openmp and openmp offload flags are utilised awarzynski wrote: > agozillon wrote: > >

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

agozillon added inline comments. Comment at: clang/test/Driver/flang/flang-omp.f90:1 +! Check that flang -fc1 is invoked when in --driver-mode=flang +! and the relevant openmp and openmp offload flags are utilised awarzynski wrote: > agozillon wrote: > >

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

agozillon added inline comments. Comment at: clang/test/Driver/flang/flang-omp.f90:1 +! Check that flang -fc1 is invoked when in --driver-mode=flang +! and the relevant openmp and openmp offload flags are utilised awarzynski wrote: > agozillon wrote: > >

[PATCH] D145264: [OpenMP][MLIR] Lower and apply RTLModuleFlagsAttr

agozillon added a comment. This builds on the following phabricator patches that are in review at the moment, so there are components that are used here that are added from these: https://reviews.llvm.org/D144864 : add fopenmp-is-device and an mlir is-device attribute for the OpenMP dialect

[PATCH] D145264: [OpenMP][MLIR] Lower and apply RTLModuleFlagsAttr

agozillon created this revision. Herald added subscribers: Moerafaat, zero9178, bzcheeseman, awarzynski, sdasgup3, wenzhicui, wrengr, cota, teijeong, rdzhabarov, tatianashp, msifontes, jurahul, Kayjukh, grosul1, Joonsoo, liufengdb, aartbik, mgester, arpith-jacob, antiagainst, shauheen, rriddle,

  1   2   >