This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Fix declare target link implementation
ClosedPublic

Authored by gtbercea on Jul 11 2019, 1:50 PM.

Details

Summary

This patch fixes the case where variables in different compilation units or the same compilation unit are under the declare target link clause AND have the same name.
This also fixes the name clash error that occurs when unified memory is activated.
The changes in this patch include:

  • Pointers to internal variables are given unique names.
  • Externally visible variables are given the same name as before.
  • All pointer variables (external or internal) are weakly linked.

Diff Detail

Event Timeline

gtbercea created this revision.Jul 11 2019, 1:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 11 2019, 1:50 PM

Description lacking.
Why is that bad?
How does this play wrt reproducibility of the output?
Normally value names in IR are completely discarded in Release build mode, why do they matter here?

ABataev added inline comments.Jul 11 2019, 2:12 PM
lib/CodeGen/CGOpenMPRuntime.cpp
2593
  1. Capitalize the first letter in tbe variable name.
  2. Why do you need to use hash? Could you use variable sourcelocation instead?

Also, what if the variable is defined in one file but declared in another one file. Will it be linked correctly?

lib/Sema/SemaOpenMP.cpp
2625 ↗(On Diff #209319)

I assume the changes in this file are from the different fix.

gtbercea updated this revision to Diff 210386.Jul 17 2019, 12:01 PM
  • Fix tests.
gtbercea marked an inline comment as done.Jul 17 2019, 12:03 PM
gtbercea edited the summary of this revision. (Show Details)Jul 17 2019, 12:06 PM
ABataev added inline comments.Jul 17 2019, 12:09 PM
lib/CodeGen/CGOpenMPRuntime.cpp
2609

Better to fix the link clause processing in a different patch, it has nothing to do with the unified memory.

gtbercea marked an inline comment as done.Jul 17 2019, 12:15 PM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.cpp
2609

Sure I can split this patch.

gtbercea marked an inline comment as done.Jul 18 2019, 9:08 AM
gtbercea added inline comments.
lib/CodeGen/CGOpenMPRuntime.cpp
2609

It turns out that the patch cannot be split. Splitting the patch would lead to an intermediate state which produced incorrect results for certain corner cases.

gtbercea marked an inline comment as done.Jul 18 2019, 9:08 AM
gtbercea retitled this revision from [OpenMP] Fix unified memory implementation for multiple compilation units to [OpenMP] Fix target link implementation.Jul 18 2019, 9:17 AM
gtbercea edited the summary of this revision. (Show Details)
ABataev added inline comments.Jul 18 2019, 9:26 AM
lib/CodeGen/CGOpenMPRuntime.cpp
2589

Currently, I think, it would better to use isExternallyVisible() member function of VD variable.

2606–2613

Remove braces

2612–2613

Remove this, it is not required anymore since the pointers have weakany linkage

test/OpenMP/declare_target_codegen.cpp
23–26

I would not rely on the fact that the FileId is always 0

gtbercea updated this revision to Diff 210645.Jul 18 2019, 12:09 PM
  • Address comments.
gtbercea marked 3 inline comments as done.Jul 18 2019, 12:09 PM

Fix the name of the patch. It is the patch for declare target, not target.

lib/CodeGen/CGOpenMPRuntime.cpp
2598–2599

Why do you want to add to zeroed FileId for externally visible vars? Leave them as-is.

gtbercea retitled this revision from [OpenMP] Fix target link implementation to [OpenMP] Fix declare target link implementation.Jul 18 2019, 12:17 PM
gtbercea updated this revision to Diff 210650.Jul 18 2019, 12:31 PM
  • Fix tests. Address comments.
ABataev added inline comments.Jul 18 2019, 12:36 PM
lib/CodeGen/CGOpenMPRuntime.cpp
2594–2597

Merge these 2 checks into one, please.

OS << CGM.getMangledName(GlobalDecl(VD));
if (!VD->isExternallyVisible()) {
      unsigned DeviceID, Line;
      unsigned FileID = 0;
        getTargetEntryUniqueInfo(CGM.getContext(),
                                 VD->getCanonicalDecl()->getBeginLoc(),
                                 DeviceID, FileID, Line);
       OS << llvm::format("_%x", FileID);
}
OS << "_decl_tgt_ref_ptr";
gtbercea updated this revision to Diff 210652.Jul 18 2019, 12:42 PM
  • Merge conditionals.
gtbercea marked 3 inline comments as done.Jul 18 2019, 12:43 PM

I don't see a single test for static variables. Do we have at least one? If not, add it.

gtbercea updated this revision to Diff 210673.Jul 18 2019, 2:14 PM
  • Add static variable.
ABataev added inline comments.Jul 19 2019, 7:06 AM
lib/CodeGen/CGOpenMPRuntime.cpp
2592–2601

Move those 3 vars to if statement, they are not needed here.

test/OpenMP/declare_target_link_codegen.cpp
22–23

Better to define the variable with the name of the original d variable for better and stable check

gtbercea marked an inline comment as done.Jul 19 2019, 9:21 AM
gtbercea added inline comments.
test/OpenMP/declare_target_link_codegen.cpp
22–23

Can you rephrase your comment please? I don't understand what you'd like me to do here.

Do you want me to use [[...]] to define the variable?

gtbercea updated this revision to Diff 210854.Jul 19 2019, 9:30 AM
  • Address comments.
gtbercea marked 2 inline comments as done.Jul 19 2019, 9:31 AM

LG with a nit.

lib/CodeGen/CGOpenMPRuntime.cpp
2595

No need to initialize FileID.

gtbercea updated this revision to Diff 210856.Jul 19 2019, 9:40 AM
  • Address comments.
gtbercea marked an inline comment as done.Jul 19 2019, 9:41 AM
ABataev accepted this revision.Jul 26 2019, 11:01 AM
This revision is now accepted and ready to land.Jul 26 2019, 11:01 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2019, 2:15 PM