Page MenuHomePhabricator

[Clang][OpenMP Offload] Move offload registration code to the wrapper
ClosedPublic

Authored by sdmitriev on Oct 9 2019, 5:05 PM.

Details

Summary

The final list of OpenMP offload targets becomes known only at the link time and since offload registration code depends on the targets list it makes sense to delay offload registration code generation to the link time instead of adding it to the host part of every fat object. This patch moves offload registration code generation from clang to the offload wrapper tool.

This is the last part of the OpenMP linker script elimination patch https://reviews.llvm.org/D64943

Diff Detail

Event Timeline

sdmitriev created this revision.Oct 9 2019, 5:05 PM
ABataev added inline comments.Oct 10 2019, 7:32 AM
clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
74

Same question as before: maybe better to make the size of size_t type a parameter of a tool?

174

Why ArrayRef<char>, not StringRef? It has a constructor for pointer/length pair.

204

No need for const and & here.

sdmitriev added inline comments.Oct 10 2019, 2:33 PM
clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
74

As I remember you also had another suggestion - change size_t to intptr_t. That will eliminate the need to an additional parameter for size type. Will it be better?

204

Right.

sdmitriev added inline comments.Oct 10 2019, 2:56 PM
clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
174

Well, in this context StringRef type would be similar to ArrayRef<char>, but with extra operations for string manipulations. Since we know that device images are not strings, we do not really need any string operations for image buffers and therefore I think ArrayRef<char> is a better choice here.

ABataev added inline comments.Oct 10 2019, 3:12 PM
clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
74

In thi case we'll need to change the type in the libomptarget.

sdmitriev added inline comments.
clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
74

Right. @grokos , do you see any potential problems in changing __tgt_offload_entry::size type from size_t to intptr_t?

grokos added inline comments.Oct 10 2019, 5:05 PM
clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
74

As long as intptr_t has the same size as size_t it should be fine. Of course, if this is not the case, then if libomptarget tries to load an older image where sizeof(__tgt_offload_entry::size) != sizeof(intptr_t) then backwards compatibility will have been broken. Fortunately, on all platforms supported by released versions of libomptarget so far (x86_64, ppc64, aarch64) if I'm not mistaken sizeof(size_t) == sizeof(initptr_t), so I don't think we'll break anything.

sdmitriev marked an inline comment as done.Oct 11 2019, 8:47 AM
sdmitriev added inline comments.
clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
74

@ABataev, as I understand clang CG assumes that intptr_t, size_t, and ptrdiff_t are the same types.
Please look at clang/lib/CodeGen/CodeGenTypeCache.h, lines 44-49

/// intptr_t, size_t, and ptrdiff_t, which we assume are the same size.
union {
  llvm::IntegerType *IntPtrTy;
  llvm::IntegerType *SizeTy;
  llvm::IntegerType *PtrDiffTy;
};

So, I guess what we have here is Ok.

ABataev added inline comments.Oct 11 2019, 9:11 AM
clang/tools/clang-offload-wrapper/ClangOffloadWrapper.cpp
74

Ok, fix at least other comment(s)

sdmitriev updated this revision to Diff 224622.Oct 11 2019, 9:54 AM
sdmitriev marked 8 inline comments as done.
sdmitriev edited the summary of this revision. (Show Details)

Rebased patch and addressed review comments.

This revision is now accepted and ready to land.Oct 11 2019, 10:05 AM
This revision was automatically updated to reflect the committed changes.