Sorry for the delay. LGTM.

Thank you all for the reviews! I will upload the final changes shortly and merge them after pre-merge checks pass.

Latest revision is rebase + Lower/array-expression-slice-1.f90 fix.

@awarzynski, thank you for the review!

In D128385#3608828, @tschuett wrote:

It would be nice if you could lower most of the Fortran math intrinsics to the Math dialect. You can extend it as needed. LLVM backends can then decide whether they need libcalls.

It should give you some optimisation opportunities in MLIR.

Thank you for the thorough review! Please see my comments below.

Thank you for reviewing!

In D128385#3604073, @kiranchandramohan wrote:

+1. Yes, that would be a fast way to get that portion of the patch in.

Please review.

If there are no more comments, I am going to merge this by the end of the week.

In D108303#2952393, @JonChesterfield wrote:

I like D108246 more. None of the offloading tests updated in D108246 failed with the above patch, perhaps they're not run by make check-openmp

Hi Jon, I am about to merge D108246 that disables the notes embedding.

In D99551#2952336, @JonChesterfield wrote:

Nvptx broken here too, amdgpu is fine. I'm guessing one of the cuda tools does some overly aggressive input validation that we're running afoul of.

There was a discussion about this on the call today - plan was to put it behind a disabled boolean argument while fixing to avoid downstream churn. Sadly the original patch was not authored with that in mind. I suggest if we can't get that patch together asap we revert this and fix it offline (even if the fix is adding said flag)

In D108246#2950650, @jdoerfert wrote:

I would prefer to revert 93d08acaacec951dbb302f77eeae51974985b6b2 now and then fix the CUDA toolchain rather than making it someone else's problem.

In D99551#2950522, @jhuber6 wrote:

In D99551#2950515, @vzakhari wrote:

In D99551#2950464, @jhuber6 wrote:

In D99551#2950281, @vzakhari wrote:

@jhuber6, thank you for reporting this. I do not have a properly setup CUDA system currently. Can you please invoke clang-offload-wrapper with -save-temps and send the temporary files to me?

I sent it to you, any luck or should we revert this upstream for the time being.

Unfortunately, I did not receive it. Can you please check if the mail was blocked on your side? Can you please try to archive it with a password and send it again?
I would like to prepare a patch to put this functionality under a switch that is off by default. I think we want to have this working eventually (and it actually works with x86_64 offload), so it will be easier to just flip a switch, when I figure out what CUDA API does not like about the modified ELF image. Does it sound appropriate to you?

I put it in a .tar.gz file, I think your domain is blocking it. Anything else I can use?

In D99551#2950464, @jhuber6 wrote:

In D99551#2950281, @vzakhari wrote:

@jhuber6, thank you for reporting this. I do not have a properly setup CUDA system currently. Can you please invoke clang-offload-wrapper with -save-temps and send the temporary files to me?

I sent it to you, any luck or should we revert this upstream for the time being.

@jhuber6, thank you for reporting this. I do not have a properly setup CUDA system currently. Can you please invoke clang-offload-wrapper with -save-temps and send the temporary files to me?

In the latest update I added llvm-objcopy as a dependence for clang-offload-wrapper, because otherwise make check-libomptarget would fail.

In D99553#2935395, @JonChesterfield wrote:

The commit message is out of date.

The implementation is very clear. Thank you for the ping, missed the patch update email.

Edit: appears I had some inline comments from a previous revision that I had not posted, please disregard them.

ping...
Can someone please review the updated (simplified) patch?

LGTM

In D103545#2827091, @tianshilei1992 wrote:

I build my LLVM with the command:

cmake -G Ninja -DCMAKE_INSTALL_PREFIX=$HOME/Documents/deploy/llvm/release -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang" -DLLVM_INSTALL_UTILS=ON -DLLVM_INCLUDE_BENCHMARKS=OFF -DLLVM_INCLUDE_TESTS=ON -DLLVM_INCLUDE_EXAMPLES=OFF -DLLVM_ENABLE_ASSERTIONS=ON $HOME/Documents/vscode/llvm-project/llvm

And then have a separate configure for OpenMP:

cmake -G Ninja -DCMAKE_INSTALL_PREFIX=$HOME/Documents/deploy/openmp/debug -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER=$HOME/Documents/deploy/llvm/release/bin/clang -DCMAKE_CXX_COMPILER=$HOME/Documents/deploy/llvm/release/bin/clang++ -DLIBOMPTARGET_NVPTX_COMPUTE_CAPABILITIES=75 -DOPENMP_NOT_EXECUTABLE=$HOME/Documents/deploy/llvm/release/bin/not -DCUDA_TOOLKIT_ROOT_DIR=/usr/local/cuda -DLLVM_ROOT=$HOME/Documents/deploy/llvm/release -DLIBOMPTARGET_LLVM_INCLUDE_DIRS=$HOME/Documents/vscode/llvm-project/llvm/include $HOME/Documents/vscode/llvm-project/openm

In D103545#2827177, @ronlieb wrote:

Thank you, @ronlieb! I am able to reproduce it. It seems that LLVM_ENABLE_RUNTIMES behavior is quite different from the -DLLVM_ENABLE_PROJECTS="openmp" that I use (so I did not see this issue before). It seems to me that in LLVM_ENABLE_RUNTIMES mode, openmp is configured/built as a standalone project using clang built by the "main" cmake/build step, but for some reason OPENMP_STANDALONE_BUILD is false. I am not sure about the right solution yet. Can you please add openmp to your -DLLVM_ENABLE_PROJECTS="clang;lld" and remove "-DLLVM_ENABLE_RUNTIMES="openmp" as a temporary workaround?

i added openmp to PROJECTS, and removed -DLLVM_ENABLE_RUNTIMES="openmp"

results in error

CMake Error at cmake/modules/LLVMExternalProjectUtils.cmake:358 (add_custom_target):

add_custom_target cannot create target "check-openmp" because another
target with the same name already exists.  The existing target is a custom
target created in source directory
"/work/rlieberm/mono-repo/llvm-project/openmp".  See documentation for
policy CMP0002 for more details.

Call Stack (most recent call first):

runtimes/CMakeLists.txt:226 (llvm_ExternalProject_Add)
runtimes/CMakeLists.txt:353 (runtime_default_target)

In D103545#2826828, @ronlieb wrote:

i forgot to mention that the first problem i encountered with this patch is that elf_common.cpp has difficulties finding its LLVM header files.
i am reporting that issue. For me, the fPIC relocatables might be a downstream issue, still looking into that one.

for the header file issue, i cobbled this up
add to end of file openmp/libomptarget/plugins/common/elf_common/CMakeLists.txt
include_directories(${LIBOMPTARGET_LLVM_INCLUDE_DIRS})

and added a cmake definition kind of like:
-DLIBOMPTARGET_LLVM_INCLUDE_DIRS=/<blah>/llvm-project/llvm/include

A person more versed in CMake might have a better solution, hope so.

In D103545#2826676, @ronlieb wrote:

[AMD Official Use Only]

Trunk as of this morning
work/rlieberm/mono-repo/llvm-project/openmp/libomptarget/plugins/common/elf_common/elf_common.cpp
/work/rlieberm/mono-repo/llvm-project/openmp/libomptarget/plugins/common/elf_common/elf_common.cpp:15:10: fatal error: 'llvm/BinaryFormat/Magic.h' file not found
#include "llvm/BinaryFormat/Magic.h"

^~~~~~~~~~~~~~~~~~~~~~~~~~~

1 error generated.
[182/204] Building CXX object openmp/tools/archer/CMakeFiles/archer.dir/ompt-tsan.cpp.o

Cmake command, you can probably drop the CMAKE_INSTALL_PREFIX

cmake -DCMAKE_INSTALL_PREFIX=/home/rlieberm/rocm/trunk_1.0 \

-DCMAKE_BUILD_TYPE=Release \
-DLLVM_ENABLE_PROJECTS="clang;lld" \
-DLLVM_TARGETS_TO_BUILD="X86;AMDGPU" \
-DLLVM_ENABLE_ASSERTIONS=ON                        \
-DLLVM_ENABLE_RUNTIMES="openmp" \
-DCLANG_DEFAULT_LINKER=lld                         \
../llvm -GNinja

Can you please provide the CMake command to reproduce this? I cannot reproduce this with an ordinary in-tree build.

In D103545#2821456, @JonChesterfield wrote:

Are you expecting to land this soon?

Just a rebase.

Implemented using lib/BinaryFormat/Magic.cpp

In D103546#2802020, @jhenderson wrote:

In D103546#2799634, @vzakhari wrote:

In D103546#2799569, @MaskRay wrote:

We have lib/BinaryFormat/Magic.cpp. Do we need the ELF specific API?

Thanks! I was not aware of this code. How about adding something like static inline is_any_elf(file_magic) method in Magic.h? If you agree, then I will abandon this change-set and add the new method in D103545.

I'm assuming this would be to avoid having to write something like if (objectType == elf || objectType == elf_relocatable || /*all the other elf_* types*/ ...) (or an equivalent switch statement)? Is this something you'd use in more than one place within your code?

In D103546#2799569, @MaskRay wrote:

We have lib/BinaryFormat/Magic.cpp. Do we need the ELF specific API?

In D101341#2781231, @jhenderson wrote:

In D101341#2755605, @vzakhari wrote:

In D101341#2753240, @jhenderson wrote:

@vzakhari, what's the motivation for doing this? These data structures are used within yaml2obj/obj2yaml, although I'm not particularly familiar with the plumbing to identify whether it matters. My suspicion is that it doesn't.

@jhenderson, there are two ways to create a YAML file using YAML mappings. The first one is to create a YAML file from a string bufffer (see line 38 in llvm/unittests/ObjectYAML/ELFYAMLTest.cpp). The second way is to populate the ELFYAML:: data structures and stream them into yaml::Output. Both ways are valid. The second way is a little bit more convenient for programmatic ELF-YAML creation in my downstream application. When I try to use ELFYAML:: data structures, there is no convenient way to make sure that all optional members in them are initialized. Neither default nor zero-initialization works. This is why I made these changes to make sure that all optional members are initialized properly. Note that the default values are not necessarily zeroes.

Which cases don't have zero as their default value? The ones you've modified in this patch all are (even the Mips enums are - the enum values are value 0).

Sorry, I do not see where I said that there are no default values. I said that neither default nor zero-initialization works - in this phrase I referred to initialization of POD members of objects, when we use default or zero-initialization. For example, look at struct MipsABIFlags: if we declare an object of this type using default initialization/construction, then members like ISARevision will be default constructed, which in turn causes their value to be undefined (because they use the default constructor of the type defined by LLVM_YAML_STRONG_TYPEDEF). Value/zero-initialization will also not work for such objects, because struct MipsABIFlags has a user-defined constructor.

ping