[PATCH] D115586: [NFC][Clang][OpenMP] Use switch-case statement to process clauses of atomic directive

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115586/new/ https://reviews.llvm.org/D115586 ___

[PATCH] D115888: [Attributor][Fix] Add alignment return attribute to HeapToStack

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntimeGPU.cpp:1411 +CGM.getLLVMContext(), llvm::Attribute::Alignment, +CGM.getContext().getTypeAlignInChars(VarTy).getQuantity())); This doesn't work. If the type alignment

[PATCH] D115888: [Attributor][Fix] Add alignment return attribute to HeapToStack

jdoerfert accepted this revision. jdoerfert added a comment. LG, don't forget to update the commit message. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115888/new/ https://reviews.llvm.org/D115888 ___

[PATCH] D115998: [Clang] Add helper text to fopenmp_version_EQ to make it show in help menu

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115998/new/ https://reviews.llvm.org/D115998 ___

[PATCH] D102107: [OpenMP] Codegen aggregate for outlined function captures

jdoerfert accepted this revision. jdoerfert added a comment. Can we land this? AMD issues seems resolved. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D102107/new/ https://reviews.llvm.org/D102107 ___ cf

[PATCH] D115971: [OpenMP][FIX] Change globalization alignment to 16

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LG, partial fix for the problem and fixmes describing what needs to be done next. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115971/new

[PATCH] D115888: [Attributor][Fix] Add alignment return attribute to HeapToStack

jdoerfert added a comment. Still LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115888/new/ https://reviews.llvm.org/D115888 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.o

[PATCH] D113126: [OpenMP][NFCI] Embed the source location string size in the ident_t

jdoerfert added a comment. In D113126#3211935 , @tianshilei1992 wrote: > In D113126#3122947 , @jdoerfert > wrote: > >> In D113126#3122659 , >> @tianshilei1992 wrote: >>

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LG, it unbreaks the build and is not totally bananas. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98746/new/ https://reviews.llvm.org/D9

[PATCH] D98746: [clang][amdgpu] Use implicit code object default

jdoerfert added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:1115-1124 + // Currently defaults to 3 in AMDGPUBaseInfo.cpp + // Using that default lets clang emit IR for amdgcn when llvm has been built + // without that target, provided the user wants this

[PATCH] D98902: [Clang][OpenMP][NVPTX] Fixed failure in openmp-offload-gpu.c if the system has CUDA

jdoerfert added a comment. @tra, so you think we should not do this? The user will see a link error late I assume, might be better. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98902/new/ https://reviews.llvm.org/D98902 _

[PATCH] D99280: [OPENMP]Fix PR48571: critical/master in outlined contexts cause crash.

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LG, thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99280/new/ https://reviews.llvm.org/D99280 _

[PATCH] D99288: [OPENMP]Fix PR49468: Declare target should allow empty sequences and namespaces.

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM, thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99288/new/ https://reviews.llvm.org/D99288 ___

[PATCH] D99372: [OpenMP][NFC] Generate check lines with update script

jdoerfert created this revision. jdoerfert added reviewers: AMDChirag, fghanim, ggeorgakoudis, ABataev. Herald added subscribers: jfb, guansong, yaxunl. Herald added a reviewer: bollu. jdoerfert requested review of this revision. Herald added a subscriber: sstefan1. Herald added a project: clang.

[PATCH] D99415: [Utils] Add prefix parameter in update test checks to avoid FileCheck conflicts

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. Cool, thanks. LGTM, two nits. Comment at: llvm/utils/UpdateTestChecks/common.py:532 + and _prefix_filecheck_ir_name: + var = _prefix_filecheck_ir_name + var

[PATCH] D99372: [OpenMP][NFC] Generate check lines with update script

jdoerfert added inline comments. Comment at: clang/test/OpenMP/cancel_codegen.cpp:83 +// OMP-LABEL: @main( +// OMP-NEXT: entry: +// OMP-NEXT:[[RETVAL:%.*]] = alloca i32, align 4 ABataev wrote: > I think it may fail in release mode, where no symbol names are

[PATCH] D99432: [OPENMP]Fix PR48851: the locals are not globalized in SPMD mode.

jdoerfert added a comment. Can we please always do the globalization, even in the `target teams distribute parallel for` case you need it if a thread shares the address of a local variable with the team and another thread uses it. There is no argument other than "doesn't escape" that Clang can m

[PATCH] D99433: [Matrix] Including __builtin_matrix_multiply_add for the matrix type extension.

jdoerfert added a comment. [Drive by] LLVM test missing? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99433/new/ https://reviews.llvm.org/D99433 ___ cfe-commits mailing list cfe-commits@lists.llvm.org h

[PATCH] D99433: [Matrix] Including __builtin_matrix_multiply_add for the matrix type extension.

jdoerfert added a comment. In D99433#2653528 , @everton.constantino wrote: > @jdoerfert Which tests do you have in mind? I added one for SEMA and one for > CodeGen. Tests for everything you placed in `llvm/`. Your tests are all in `clang/`. =

[PATCH] D99445: [OPENMP]Fix PR49052: Clang crashed when compiling target code with assert(0).

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99445/new/ https://reviews.llvm.org/D99445 _

[PATCH] D99297: [OPENMP]Fix PR49636: Assertion `(!Entry.getAddress() || Entry.getAddress() == Addr) && "Resetting with the new address."' failed.

jdoerfert accepted this revision. jdoerfert added a subscriber: tra. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM, @tra might have input on the cuda change but it seems reasonable and we can do that post commit. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D99432: [OPENMP]Fix PR48851: the locals are not globalized in SPMD mode.

jdoerfert added a comment. In D99432#2653483 , @ABataev wrote: > In D99432#2653474 , @jdoerfert wrote: > >> Can we please always do the globalization, even in the `target teams >> distribute parallel for` case you

[PATCH] D99506: [OpenMP][NFC] Move the `noinline` to the parallel entry point

jdoerfert created this revision. jdoerfert added reviewers: tianshilei1992, ye-luo, JonChesterfield. Herald added subscribers: guansong, yaxunl. Herald added a reviewer: bollu. jdoerfert requested review of this revision. Herald added subscribers: cfe-commits, sstefan1. Herald added a project: clan

[PATCH] D99506: [OpenMP][NFC] Move the `noinline` to the parallel entry point

jdoerfert added a comment. In D99506#2655948 , @fhahn wrote: > Is it possible to add a test? There is a test for the presence of noinline, let me make it more explicit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D99506: [OpenMP][NFC] Move the `noinline` to the parallel entry point

jdoerfert updated this revision to Diff 333895. jdoerfert added a comment. Add test for nvptx codegen, including wrapper attribute check Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99506/new/ https://reviews.llvm.org/D99506 Files: clang/lib/Co

[PATCH] D99539: [OPENMP]Fix PR48740: OpenMP declare reduction in C does not require an initializer

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99539/new/ https://reviews.llvm.org/D99539 _

[PATCH] D99506: [OpenMP][NFC] Move the `noinline` to the parallel entry point

This revision was automatically updated to reflect the committed changes. Closed by commit rG03cc8a1ba050: [OpenMP][NFC] Move the `noinline` to the parallel entry point (authored by jdoerfert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99506/new/

[PATCH] D99617: [OPENMP]Fix PR48607: Crash during clang openmp codegen for firstprivate() of `float _Complex`.

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LG, one nit. Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:644 + if (PI->getType()->isFirstClassType()) +; LValue LV = WrapperCGF.MakeAddrLValue( -

[PATCH] D99611: [OPENMP]Fix PR48658: [OpenMP 5.0] Compiler crash when OpenMP atomic sync hints used.

jdoerfert added inline comments. Comment at: clang/test/OpenMP/atomic_codegen.cpp:61 St().get() %= b; -#pragma omp atomic +#pragma omp atomic hint(6) s.field++; Should this influence the IR in any way? Repository: rG LLVM Github Monorepo CHANGE

[PATCH] D99611: [OPENMP]Fix PR48658: [OpenMP 5.0] Compiler crash when OpenMP atomic sync hints used.

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. Hm, OK. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99611/new/ https://reviews.llvm.org/D99611

[PATCH] D99521: [OPENMP]Fix PR48885: Crash in passing firstprivate args to tasks on Apple M1.

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM. I assume we can soon get rid of the variadic function type if we never use it as such. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/

[PATCH] D99689: [OPENMP]Add option -fopenmp-cuda-const-firstprivate to control address space of the corresponding global.

jdoerfert added a comment. Can you please show me a test case or explain to me when/how this global is actually used. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99689/new/ https://reviews.llvm.org/D99689 ___

[PATCH] D99689: [OPENMP]Add option -fopenmp-cuda-const-firstprivate to control address space of the corresponding global.

jdoerfert added a comment. In D99689#2662852 , @ABataev wrote: > In D99689#2662848 , @jdoerfert wrote: > >> Can you please show me a test case or explain to me when/how this global is >> actually used. > > It is pa

[PATCH] D99689: [OPENMP]Add option -fopenmp-cuda-const-firstprivate to control address space of the corresponding global.

jdoerfert added a comment. In D99689#2662860 , @ABataev wrote: > In D99689#2662856 , @jdoerfert wrote: > >> In D99689#2662852 , @ABataev wrote: >> >>> In D99689#2662848

[PATCH] D99681: [OpenMP] Pass mapping names to add components in a user defined mapper

jdoerfert added a comment. A test please. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99681/new/ https://reviews.llvm.org/D99681 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.ll

[PATCH] D99681: [OpenMP] Pass mapping names to add components in a user defined mapper

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99681/new/ https://reviews.llvm.org/D99681 ___

[PATCH] D99537: [OPENMP51]Initial support for the dispatch directive

jdoerfert added a subscriber: jyu2. jdoerfert added a comment. Cool, are you expecting to hook this up to the variant selection logic as well? @jyu2 similarly do you intend to hook up `novariant`? We should also update https://clang.llvm.org/docs/OpenMPSupport.html to claim parts you are workin

[PATCH] D99447: [OpenMP] Define omp_is_initial_device() variants in omp.h

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LG, thanks Comment at: openmp/runtime/src/include/omp.h.var:479 +# endif + # undef __KAI_KMPC_CONVENTION hbae wrote: > jdoerfert wrote: > > Do we

[PATCH] D99949: [AMDGPU][OpenMP] Add amdgpu-arch tool to list AMD GPUs installed

jdoerfert added a comment. no strong opinion rn, I would add tests though Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99949/new/ https://reviews.llvm.org/D99949 ___ cfe-commits mailing list cfe-commits

[PATCH] D100507: [Clang][Docs] Claim the atomic compare

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LG Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100507/new/ https://reviews.llvm.org/D100507 ___

[PATCH] D100609: [Offload][OpenMP][CUDA] Allow fembed-bitcode for device offload

jdoerfert created this revision. jdoerfert added a reviewer: tra. Herald added subscribers: guansong, yaxunl. Herald added a reviewer: bollu. jdoerfert requested review of this revision. Herald added a subscriber: sstefan1. Herald added a project: clang. This is a fix for the problem reported here

[PATCH] D100609: [Offload][OpenMP][CUDA] Allow fembed-bitcode for device offload

jdoerfert added a comment. I'm not really sure about the test, my local setup didn't have CUDA attached properly but this should work in principle ;) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100609/new/ https://reviews.llvm.org/D100609 _

[PATCH] D95976: [OpenMP] Simplify offloading parallel call codegen

jdoerfert added a comment. I have only minor remarks but I'd like you to check if my hunch is correct and the proposed modifications will fix fix PR49777 *and* fix PR49779. Also, the number of arguments need to be increased, let's go big and automatic here. Other than that I think this looks go

[PATCH] D100514: [OpenMP] Added codegen for masked directive

jdoerfert added a comment. Any reason we should not unconditionally use the OMPIRBuilder impl? (btw, many thanks for providing one!) We have an OMPIRBuilder always around in clang's codegen, so there is little reason not to use it if it is feature complete. Repository: rG LLVM Github Monorep

[PATCH] D100514: [OpenMP] Added codegen for masked directive

jdoerfert added a comment. Also, don't forget to mark it as done in https://clang.llvm.org/docs/OpenMPSupport.html :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100514/new/ https://reviews.llvm.org/D100514 _

[PATCH] D100620: [OpenMP] Make sure classes work on the device as they do on the host

jdoerfert created this revision. jdoerfert added reviewers: JonChesterfield, ABataev, grokos. Herald added subscribers: guansong, yaxunl. Herald added a reviewer: bollu. jdoerfert requested review of this revision. Herald added subscribers: cfe-commits, sstefan1. Herald added a project: clang. We

[PATCH] D100621: [OpenMP] Ensure the DefaultMapperId has a location

jdoerfert created this revision. jdoerfert added reviewers: JonChesterfield, ABataev, grokos. Herald added subscribers: guansong, yaxunl. Herald added a reviewer: bollu. jdoerfert requested review of this revision. Herald added subscribers: cfe-commits, sstefan1. Herald added a project: clang. A u

[PATCH] D100514: [OpenMP] Added codegen for masked directive

jdoerfert added a comment. In D100514#2695271 , @cchen wrote: > In D100514#2693600 , @jdoerfert > wrote: > >> Any reason we should not unconditionally use the OMPIRBuilder impl? (btw, >> many thanks for providin

[PATCH] D100609: [Offload][OpenMP][CUDA] Allow fembed-bitcode for device offload

jdoerfert added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:4442-4446 + std::string CPU = getCPUName(Args, Triple, /*FromAs*/ false); + if (!CPU.empty()) { +CmdArgs.push_back("-target-cpu"); +CmdArgs.push_back(Args.MakeArgString(CP

[PATCH] D100673: [OPENMP]Fix PR49698: OpenMP declare mapper causes segmentation fault.

jdoerfert added a comment. Can we use the reproducer from the bug report, I want an outermost array section with objects that have a declare mapper. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100673/new/ https://reviews.llvm.org/D100673 __

[PATCH] D100673: [OPENMP]Fix PR49698: OpenMP declare mapper causes segmentation fault.

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LG, thanks for the quick fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100673/new/ https://reviews.llvm.org/D100673

[PATCH] D100609: [Offload][OpenMP][CUDA] Allow fembed-bitcode for device offload

jdoerfert added inline comments. Comment at: clang/test/Driver/embed-bitcode-nvptx.cu:1 +// RUN: %clang -Xclang -triple -Xclang nvptx64 -S -Xclang -target-feature -Xclang +ptx70 -fembed-bitcode=all --cuda-device-only -nocudalib -nocudainc %s -o - | FileCheck %s +// REQUIRES: nv

[PATCH] D100620: [OpenMP] Make sure classes work on the device as they do on the host

jdoerfert added inline comments. Comment at: clang/lib/Headers/openmp_wrappers/__clang_openmp_device_functions.h:20 +// need to `include `. +#include +#endif JonChesterfield wrote: > I think there are legitimate use cases for writing code that doesn't include

[PATCH] D95976: [OpenMP] Simplify offloading parallel call codegen

jdoerfert accepted this revision. jdoerfert added a comment. With the nit to add the two reproducers, LGTM. (please make sure to run FAROS or some benchmarks we have before commiting). Comment at: openmp/libomptarget/deviceRTLs/common/src/parallel.cu:294 + // TODO: Add UNLIKE

[PATCH] D100514: [OpenMP] Added codegen for masked directive

jdoerfert added a comment. In D100514#2696106 , @cchen wrote: >> Initially we did not have an OMPIRBuilder object unconditionally, now we >> have. Let's move over everything that is ready. So master and critical >> should be good to go as well I suppose

[PATCH] D100874: [OpenMP] Use irbuilder as default for masked and master construct

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM, thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100874/new/ https://reviews.llvm.org/D100874 _

[PATCH] D100874: [OpenMP] Use irbuilder as default for masked and master construct

jdoerfert added a comment. In D100874#2702897 , @cchen wrote: > I mark this patch as "plan changed" since the assert message indicates that > something wrong in IRBuilder or Codegen, however, main branch also have the > same issue so I think this patch

[PATCH] D100929: [Clang] Allow the combination of loader_uninitialized and address spaces

jdoerfert created this revision. jdoerfert added reviewers: JonChesterfield, aaron.ballman. Herald added a reviewer: bollu. jdoerfert requested review of this revision. Herald added a project: clang. When an object is allocated in a non-default address space we do not need to check for a construct

[PATCH] D101030: [OpenMP] Overhaul `declare target` handling

jdoerfert created this revision. jdoerfert added reviewers: ABataev, grokos, dreachem, cchen, JonChesterfield. Herald added subscribers: guansong, yaxunl. Herald added a reviewer: bollu. Herald added a reviewer: aaron.ballman. jdoerfert requested review of this revision. Herald added subscribers: l

[PATCH] D93785: [OpenMP][FIX] Ensure the isa trait is evaluated last

jdoerfert added a comment. 2021 ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93785/new/ https://reviews.llvm.org/D93785 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.or

[PATCH] D94123: [NVPTX] Fix debugging information being added to NVPTX target if remarks are enabled

jdoerfert added a comment. OK with me, @tra ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94123/new/ https://reviews.llvm.org/D94123 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://list

[PATCH] D94123: [NVPTX] Fix debugging information being added to NVPTX target if remarks are enabled

jdoerfert added a comment. In D94123#2482636 , @MaskRay wrote: > If you use `arc diff`, you can obtain `Reviewed-by:` line from Phabricator. > It is more useful than `Reviewers: ` (a list of reviewers do not mean they > endorse or accept the patch) `arc

[PATCH] D94185: [OpenMP][Docs] Mark finished features as done

jdoerfert created this revision. jdoerfert added a reviewer: ABataev. Herald added subscribers: guansong, bollu, yaxunl. jdoerfert requested review of this revision. Herald added subscribers: cfe-commits, sstefan1. Herald added a project: clang. Repository: rG LLVM Github Monorepo https://revie

[PATCH] D91556: [OpenMPIRBuilder} Add capturing of parameters to pass to omp::parallel

jdoerfert added inline comments. Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:784 +} + } + 1) If we would need this, remove the Counter stuff everywhere, if you want to iterate a container: `for (const T& : Container)` 2) `BlockParents` seems to be

[PATCH] D91944: OpenMP 5.0 metadirective

jdoerfert added a comment. Are the test all passing? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91944/new/ https://reviews.llvm.org/D91944 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https

[PATCH] D86844: [LoopDeletion] Allows deletion of possibly infinite side-effect free loops

jdoerfert added a comment. In D86844#2484679 , @xbolva00 wrote: > In D86844#2484639 , @atmnpatel wrote: > >> In D86844#2484568 , @fhahn wrote: >> >>> In D86844#2481922

[PATCH] D93786: [OpenMP][Fix] Make the arch selector for x86_64 work

This revision was automatically updated to reflect the committed changes. Closed by commit rGd970a285b856: [OpenMP][Fix] Make the arch selector for x86_64 work (authored by jdoerfert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93786/new/ https:/

[PATCH] D93785: [OpenMP][FIX] Ensure the isa trait is evaluated last

This revision was automatically updated to reflect the committed changes. Closed by commit rG36c4dc9b42fe: [OpenMP][FIX] Ensure the isa trait is evaluated last (authored by jdoerfert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93785/new/ https:/

[PATCH] D94185: [OpenMP][Docs] Mark finished features as done

This revision was automatically updated to reflect the committed changes. Closed by commit rG6e7101530dae: [OpenMP][Docs] Mark finished features as done (authored by jdoerfert). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94185/new/ https://review

[PATCH] D94315: [OpenMP][FIX] Enforce a function boundary for a new data environment

jdoerfert created this revision. jdoerfert added reviewers: JonChesterfield, ABataev, sstefan1. Herald added subscribers: guansong, bollu, yaxunl. jdoerfert requested review of this revision. Herald added a project: clang. Whenever we enter a new OpenMP data environment we want to enter a function

[PATCH] D94315: [OpenMP][FIX] Enforce a function boundary for a new data environment

jdoerfert added a comment. In D94315#2487164 , @ABataev wrote: > In D94315#2487150 , @JonChesterfield > wrote: > >> I'm guessing we're using the function boundary as a compiler barrier. That >> seems fragile in th

[PATCH] D94315: [OpenMP][FIX] Enforce a function boundary for a new data environment

jdoerfert added a comment. In D94315#2487244 , @ABataev wrote: > In D94315#2487242 , @jdoerfert wrote: > >> I don't understand where `inaccessiblemem_or_argmemonly` is coming from. >> This prevents us from inlining

[PATCH] D94315: [OpenMP][FIX] Enforce a function boundary for a new data environment

jdoerfert added a comment. In D94315#2487259 , @ABataev wrote: > In D94315#2487257 , @jdoerfert wrote: > >> In D94315#2487244 , @ABataev wrote: >> >>> In D94315#2487242

[PATCH] D94315: [OpenMP][FIX] Enforce a function boundary for a new data environment

jdoerfert added a subscriber: ggeorgakoudis. jdoerfert added a comment. What we should do, as we move to the OpenMPIRBuilder, is to use runtime interfaces that match OpenMP directives. Here is the `omp parallel` one for the host: D94332 The device one should loo

[PATCH] D94315: [OpenMP][FIX] Enforce a function boundary for a new data environment

jdoerfert added a comment. In D94315#2487809 , @ABataev wrote: > In D94315#2487643 , @jdoerfert wrote: > >> What we should do, as we move to the OpenMPIRBuilder, is to use runtime >> interfaces that match OpenMP di

[PATCH] D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label

jdoerfert reopened this revision. jdoerfert added a comment. This revision is now accepted and ready to land. I think this caused a lot of problems for the Attributor tests. I get the "conflict output" warning all the time now :( These two `UpdateTestChecks` tests also generate the warning. I wo

[PATCH] D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label

jdoerfert added a comment. In D93078#2499982 , @mtrofin wrote: > In D93078#2499666 , @jdoerfert wrote: > >> I think this caused a lot of problems for the Attributor tests. I get the >> "conflict output" warning all

[PATCH] D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label

jdoerfert added a comment. In D93078#246 , @mtrofin wrote: > In D93078#245 , @jdoerfert wrote: > >> In D93078#2499982 , @mtrofin wrote: >> >>> In D93078#2499666

[PATCH] D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label

jdoerfert added a comment. In D93078#2500040 , @mtrofin wrote: > In D93078#2500032 , @jdoerfert wrote: > >> In D93078#246 , @mtrofin wrote: >> >>> In D93078#245

[PATCH] D93078: [utils] Fix UpdateTestChecks case where 2 runs differ for last label

jdoerfert added a comment. In D93078#2500122 , @mtrofin wrote: > In D93078#2500114 , @jdoerfert wrote: > >> In D93078#2500040 , @mtrofin wrote: >> >>> In D93078#2500032

[PATCH] D94806: [OpenMP] Add support for mapping names in mapper API

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94806/new/ https://reviews.llvm.org/D94806 ___

[PATCH] D94871: [Clang][OpenMP] Fixed an issue that clang crashed when compiling OpenMP program in device only mode without host IR

jdoerfert added a comment. Here is a minimal reproducer for the issue even without standalone device compilation (and I can build the situation in various other ways as well): https://godbolt.org/z/T1h9b5 Can you add it as a test, I am also curious how the IR looks like with this patch. Repos

[PATCH] D94884: [Clang][OpenMP] Include header for CUDA builtin vars into OpenMP wrapper header

jdoerfert added a reviewer: tra. jdoerfert added a comment. I can see that we want this but I guess the errors show the problem, replace `__attribute((device))` with `DEVICE` and define it based on CUDA vs OpenMP properly. Comment at: clang/lib/Headers/__clang_cuda_builtin_va

[PATCH] D91944: OpenMP 5.0 metadirective

jdoerfert added a comment. In D91944#2504924 , @protze.joachim wrote: > I found two issues with this patch regarding the `default` clause: > > - The spec does not require a default clause. I get `error: expected > expression` if I omit a default clause.

[PATCH] D91944: OpenMP 5.0 metadirective

jdoerfert added a comment. In D91944#2485167 , @jdoerfert wrote: > Are the test all passing? Can we fix the failing tests so we can merge this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91944/new/ http

[PATCH] D94871: [Clang][OpenMP] Fixed an issue that clang crashed when compiling OpenMP program in device only mode without host IR

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM, one nit in the test below. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:2944-2947 +// This could happen if the device compilation is invoked standalone.

[PATCH] D94871: [Clang][OpenMP] Fixed an issue that clang crashed when compiling OpenMP program in device only mode without host IR

jdoerfert added a comment. Oh, and fix the test. If you have -verify and no errors you need `// expected-no-diagnostics` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94871/new/ https://reviews.llvm.org/D94871

[PATCH] D94884: [Clang][OpenMP] Include header for CUDA builtin vars into OpenMP wrapper header

jdoerfert added inline comments. Comment at: clang/lib/Headers/__clang_cuda_builtin_vars.h:52 + property(get = __fetch_builtin_##NAME##_##FIELD)) unsigned int FIELD; +#endif Why do we need these __fetch_builtins (for C). They have a different name than in

[PATCH] D94871: [Clang][OpenMP] Fixed an issue that clang crashed when compiling OpenMP program in device only mode without host IR

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:2944-2947 +// This could happen if the device compilation is invoked standalone. +if (!hasTargetRegionEntryInfo(DeviceID, FileID, ParentName, LineNum)) + initializeTargetRegionEntryInf

[PATCH] D94961: [OpenMP] Add OpenMP offloading toolchain skeleton for AMDGPU

jdoerfert added a comment. Won't this just prevent us from building clang due to the missing cmake changes? We need somewhat testable chunks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94961/new/ https://reviews.llvm.org/D94961 ___

[PATCH] D94871: [Clang][OpenMP] Fixed an issue that clang crashed when compiling OpenMP program in device only mode without host IR

jdoerfert added inline comments. Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:2944-2947 +// This could happen if the device compilation is invoked standalone. +if (!hasTargetRegionEntryInfo(DeviceID, FileID, ParentName, LineNum)) + initializeTargetRegionEntryInf

[PATCH] D93525: [OpenMP] Add unbundling of archives containing bundled object files into device specific archives

jdoerfert added a comment. > At the moment this patch defines compatibility as exact string match of > bundler entry ID. > [...] > Supporting target ID requires little more work and discussion. Let's get this in first, then revisit target ID support as we need it. Repository: rG LLVM Github

[PATCH] D93525: [OpenMP] Add unbundling of archives containing bundled object files into device specific archives

jdoerfert added a comment. In D93525#2509836 , @t-tye wrote: > In D93525#2509796 , @jdoerfert wrote: > >>> At the moment this patch defines compatibility as exact string match of >>> bundler entry ID. >>> [...] >>>

[PATCH] D111218: [AMDGPU][OpenMP] Remove optnone from outlined functions

jdoerfert added a comment. What if we track down the problem instead? This will simply pop up again in O0 user code, no? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111218/new/ https://reviews.llvm.org/D111218 __

[PATCH] D110781: [CUDA] Make sure is included with original __THROW defined.

jdoerfert added a comment. I first had to read up on pop_macro -.-. Still unsure if it is OK to push it once and pop it twice, that works fine? Can't we move the string include earlier, grouped with stdlib and cmath? then we don't need to play with __THROW twice. Other than that it seems sensibl

[PATCH] D110781: [CUDA] Make sure is included with original __THROW defined.

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LG. Always including string (probably) won't hurt given the stuff we do here already. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110781

[PATCH] D110114: [OMPIRBuilder] Generate aggregate argument for parallel region outlined functions

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LG. Can be merged once the requirement is in. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110114/new/ https://reviews.llvm.org/D110114

[PATCH] D111293: [CFE][Codegen][In-progress] Remove CodeGenFunction::InitTempAlloca()

jdoerfert added a comment. > CodeGenFunction::InitTempAlloca() inits the static alloca within the entry > block which may *not* necessarily be correct always. FWIW, for all uses this was correct. The point of the function was exactly to do what you state here as "potentially incorrect". The doc

[PATCH] D111348: [OpenMP] Introduce new flags to assert thread and team usage in the runtime

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LG, one nit wrt. spelling Comment at: clang/include/clang/Basic/LangOptions.def:247-248 LANGOPT(OpenMPOptimisticCollapse , 1, 0, "Use at most 32 bits to represent th

[PATCH] D111665: [CUDA] Provide address space conversion builtins.

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. Not loving the magic constants here but I don't think we have a enum or similar right now. I also have to question the people that choose `size_t` here... we will end up with int2ptr(ptr

<    13   14   15   16   17   18   19   >