Page MenuHomePhabricator

[OpenMP] Correct generation of offloading entries
ClosedPublic

Authored by Hahnfeld on Jan 17 2018, 5:12 AM.

Details

Summary

Firstly, each offloading entry must have a unique name or the
linker will complain if there are multiple files with target
regions. Secondly, the compiler must not introduce padding so
mark the struct with a PackedAttr.

Diff Detail

Repository
rC Clang

Event Timeline

Hahnfeld created this revision.Jan 17 2018, 5:12 AM

After some investigation, I think this whole code is incorrect. We should not rely on the fact that there is no padding on some architectures and should mix the type generated by the Clang and initial value, generated by LLVM.

After some investigation, I think this whole code is incorrect. We should not rely on the fact that there is no padding on some architectures and should mix the type generated by the Clang and initial value, generated by LLVM.

I'm not sure I understand what you mean. Do you refer to auto Align = CharUnits::fromQuantity(1)?

After some investigation, I think this whole code is incorrect. We should not rely on the fact that there is no padding on some architectures and should mix the type generated by the Clang and initial value, generated by LLVM.

I'm not sure I understand what you mean. Do you refer to auto Align = CharUnits::fromQuantity(1)?

Yes, this one too. But the main problem here is that we expect some layout of structure, though it may be different.

Or, there is another one solution - mark _tgt_offload_entry record as packed.

  1. Please, mark the record as packed.
  2. Tests
lib/CodeGen/CGOpenMPRuntime.cpp
3576–3579

Use llvm::Twine here instead.

Hahnfeld marked an inline comment as done.Jan 17 2018, 8:52 AM
  1. Please, mark the record as packed.

Where and how do I need to do this? I found that PackedAttr is checked on FieldDecls but I'm not sure if we go through that code path and how I could add it...

ABataev added a comment.EditedJan 17 2018, 9:20 AM
  1. Please, mark the record as packed.

Where and how do I need to do this? I found that PackedAttr is checked on FieldDecls but I'm not sure if we go through that code path and how I could add it...

I think it is applied to structure. You can do it in CGOpenMPRuntime::getTgtOffloadEntryQTy, after the RD->completeDefinition(); statement. Use RD->addAttr(PackedAttr::CreateImplicit(C));

Hahnfeld updated this revision to Diff 130399.Jan 18 2018, 5:56 AM
Hahnfeld retitled this revision from [OpenMP] Generate unique name for offloading entries to [OpenMP] Correct generation of offloading entries.
Hahnfeld edited the summary of this revision. (Show Details)

Use llvm::Twine, add PackedAttr, and adjust regression tests to check the name of the generated constants.

This revision is now accepted and ready to land.Jan 18 2018, 7:10 AM
This revision was automatically updated to reflect the committed changes.