This is an archive of the discontinued LLVM Phabricator instance.

[OPENMP] Codegen for threadprivate variables
ClosedPublic

Authored by ABataev on Jun 3 2014, 3:08 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ABataev updated this revision to Diff 10049.Jun 3 2014, 3:08 AM
ABataev retitled this revision from to [OPENMP] Codegen for threadprivate variables.
ABataev updated this object.
ABataev edited the test plan for this revision. (Show Details)
ABataev added a subscriber: Unknown Object (MLST).

I kept getting an error in the build, which is directly traced back to this
change:
llvm[4]: Compiling CGDeclCXX.cpp for Debug+Asserts build
/nfs/terran/home/michaelw/clang-dev-new/llvm/tools/clang/lib/CodeGen/CGDeclCXX.cpp:
In member function 'void
clang::CodeGen::CodeGenModule::EmitCXXOMPThreadPrivateInitFunction(const
clang::VarDecl&, llvm::Constant*, bool, bool)':
/nfs/terran/home/michaelw/clang-dev-new/llvm/tools/clang/lib/CodeGen/CGDeclCXX.cpp:600:60:
error: no matching function for call to
'llvm::FunctionType::get(llvm::PointerType*&, <brace-enclosed initializer
list>, bool)'

/*isVarArg*/

false)->getPointerTo();

^

/nfs/terran/home/michaelw/clang-dev-new/llvm/tools/clang/lib/CodeGen/CGDeclCXX.cpp:600:60:
note: candidates are:
In file included from
/nfs/terran/home/michaelw/clang-dev-new/llvm/include/llvm/IR/Constants.h:28:0,

from

/nfs/terran/home/michaelw/clang-dev-new/llvm/include/llvm/IR/ConstantFolder.h:20,

from

/nfs/terran/home/michaelw/clang-dev-new/llvm/include/llvm/IR/IRBuilder.h:22,

from

/nfs/terran/home/michaelw/clang-dev-new/llvm/tools/clang/lib/CodeGen/CGBuilder.h:13,

from

/nfs/terran/home/michaelw/clang-dev-new/llvm/tools/clang/lib/CodeGen/CodeGenFunction.h:17,

from

/nfs/terran/home/michaelw/clang-dev-new/llvm/tools/clang/lib/CodeGen/CGDeclCXX.cpp:14:
/nfs/terran/home/michaelw/clang-dev-new/llvm/include/llvm/IR/DerivedTypes.h:105:24:
note: static llvm::FunctionType* llvm::FunctionType::get(llvm::Type*,
llvm::ArrayRef<llvm::Type*>, bool)

static FunctionType *get(Type *Result,
                     ^

/nfs/terran/home/michaelw/clang-dev-new/llvm/include/llvm/IR/DerivedTypes.h:105:24:
note: no known conversion for argument 2 from '<brace-enclosed
initializer list>' to 'llvm::ArrayRef<llvm::Type*>'

I am going to try again.

ABataev updated this revision to Diff 10545.Jun 18 2014, 3:14 AM
ABataev edited edge metadata.

Replaced initializer lists in function calls by arrays.

Michael, try updated patch. I fixed possible problems.

Best regards,

Alexey Bataev

Software Engineer
Intel Compiler Team

18 Июнь 2014 г. 13:31:56, Michael Wong писал:

I kept getting an error in the build, which is directly traced back to this
change:
llvm[4]: Compiling CGDeclCXX.cpp for Debug+Asserts build
/nfs/terran/home/michaelw/clang-dev-new/llvm/tools/clang/lib/CodeGen/CGDeclCXX.cpp:
In member function 'void
clang::CodeGen::CodeGenModule::EmitCXXOMPThreadPrivateInitFunction(const
clang::VarDecl&, llvm::Constant*, bool, bool)':
/nfs/terran/home/michaelw/clang-dev-new/llvm/tools/clang/lib/CodeGen/CGDeclCXX.cpp:600:60:
error: no matching function for call to
'llvm::FunctionType::get(llvm::PointerType*&, <brace-enclosed initializer
list>, bool)'

/*isVarArg*/

false)->getPointerTo();

^

/nfs/terran/home/michaelw/clang-dev-new/llvm/tools/clang/lib/CodeGen/CGDeclCXX.cpp:600:60:
note: candidates are:
In file included from
/nfs/terran/home/michaelw/clang-dev-new/llvm/include/llvm/IR/Constants.h:28:0,

from

/nfs/terran/home/michaelw/clang-dev-new/llvm/include/llvm/IR/ConstantFolder.h:20,

from

/nfs/terran/home/michaelw/clang-dev-new/llvm/include/llvm/IR/IRBuilder.h:22,

from

/nfs/terran/home/michaelw/clang-dev-new/llvm/tools/clang/lib/CodeGen/CGBuilder.h:13,

from

/nfs/terran/home/michaelw/clang-dev-new/llvm/tools/clang/lib/CodeGen/CodeGenFunction.h:17,

from

/nfs/terran/home/michaelw/clang-dev-new/llvm/tools/clang/lib/CodeGen/CGDeclCXX.cpp:14:
/nfs/terran/home/michaelw/clang-dev-new/llvm/include/llvm/IR/DerivedTypes.h:105:24:
note: static llvm::FunctionType* llvm::FunctionType::get(llvm::Type*,
llvm::ArrayRef<llvm::Type*>, bool)

static FunctionType *get(Type *Result,
                     ^

/nfs/terran/home/michaelw/clang-dev-new/llvm/include/llvm/IR/DerivedTypes.h:105:24:
note: no known conversion for argument 2 from '<brace-enclosed
initializer list>' to 'llvm::ArrayRef<llvm::Type*>'

I am going to try again.

http://reviews.llvm.org/D4002

These changes look good to me, but I am new to clang. I did not attempt a build with these changes applied.

ejstotzer accepted this revision.Jun 24 2014, 11:09 PM
ejstotzer added a reviewer: ejstotzer.
This revision is now accepted and ready to land.Jun 24 2014, 11:09 PM

Builds fine. Change looks OK, but as this is codegen, will need more
established eyes like Hal.

John (rjmccall), could you please review this patch?
Best regards,
Alexey Bataev.

  Original Message  
From: Alexey Bataev
Sent: Monday, September 22, 2014 10:34
To: a.bataev@hotmail.com; dgregor@apple.com; hfinkel@anl.gov; cbergstrom@pathscale.com; rjmccall@gmail.com; fraggamuffin@gmail.com; ejstotzer@gmail.com; richard@metafoo.co.uk
Reply To: reviews+D4002+public+e0b72e4194bfb02b@reviews.llvm.org
Cc: cfe-commits@cs.uiuc.edu
Subject: Re: [PATCH] [OPENMP] Codegen for threadprivate variables

Ping

http://reviews.llvm.org/D4002

It's a bit sad this community cares so little when there's good code to be merged. ‎

hfinkel added inline comments.Sep 22 2014, 2:13 AM
lib/CodeGen/CGDeclCXX.cpp
547 ↗(On Diff #10545)

Like John said for EmitOMPSerialCall, this seems like it belongs in CGOpenMPRuntime.cpp.

620 ↗(On Diff #10545)

Why not use the SourceLocation of the variable's initializer?

lib/Parse/Parser.cpp
627 ↗(On Diff #10545)

Is this a separate change?

lib/Serialization/ASTReaderDecl.cpp
2186 ↗(On Diff #10545)

This is a separate change.

Ping

lib/CodeGen/CGDeclCXX.cpp
547 ↗(On Diff #10545)

I'll rework this to remove direct calls to runtime functions.

620 ↗(On Diff #10545)

Will be fixed.

lib/Parse/Parser.cpp
627 ↗(On Diff #10545)

Nope, this is required for codegen, without it the declaration is not passed to codegen consumer.

lib/Serialization/ASTReaderDecl.cpp
2186 ↗(On Diff #10545)

Nope, it's a part of codegen, without this change codegen for construct loaded from .pch does not work.

hfinkel edited edge metadata.Sep 23 2014, 5:51 AM
  • Original Message -----

From: "Alexey Bataev" <a.bataev@hotmail.com>
To: "a bataev" <a.bataev@hotmail.com>, dgregor@apple.com, hfinkel@anl.gov, rjmccall@gmail.com,
fraggamuffin@gmail.com, ejstotzer@gmail.com, richard@metafoo.co.uk, cbergstrom@pathscale.com
Cc: cfe-commits@cs.uiuc.edu
Sent: Tuesday, September 23, 2014 5:47:21 AM
Subject: Re: [PATCH] [OPENMP] Codegen for threadprivate variables

[snip]

To be clear, regarding these changes below, is either of them separately testable? If so, please go ahead and commit them now with a test.

-Hal

Comment at: lib/Parse/Parser.cpp:627
@@ -626,4 +626,3 @@

case tok::annot_pragma_openmp:
  • ParseOpenMPDeclarativeDirective();
  • return DeclGroupPtrTy();

+ return ParseOpenMPDeclarativeDirective();

case tok::annot_pragma_ms_pointers_to_members:

hfinkel wrote:

Is this a separate change?

Nope, this is required for codegen, without it the declaration is not
passed to codegen consumer.

Comment at: lib/Serialization/ASTReaderDecl.cpp:2186
@@ -2185,3 +2185,3 @@

isa<ObjCImplDecl>(D) ||
  • isa<ImportDecl>(D))

+ isa<ImportDecl>(D) || isa<OMPThreadPrivateDecl>(D))

return true;

hfinkel wrote:

This is a separate change.

Nope, it's a part of codegen, without this change codegen for
construct loaded from .pch does not work.

http://reviews.llvm.org/D4002

ABataev updated this revision to Diff 14029.Sep 24 2014, 4:28 AM
ABataev edited edge metadata.

Update after review

hfinkel accepted this revision.Oct 7 2014, 6:47 AM
hfinkel edited edge metadata.

LGTM.

lib/CodeGen/CodeGenModule.cpp
1413 ↗(On Diff #14029)

We should add a comment here explaining the separation of concerns between what EmitGlobalVarDefinition does, what EmitOMPThreadPrivateVarDecl does, and why they're both called here).

rjmccall edited edge metadata.Oct 9 2014, 3:43 PM

Okay, the big problem here is that the AST representation for threadprivate does not make any sense. This absolutely needs to be recorded on the target declaration itself somehow. I'm sorry this wasn't caught during the patch which added the Sema work.

I would suggest doing this by adding an implicit attribute to the decl. In order to work correctly with PCH/modules, you will potentially need to serialize what's called an "update record" to ensure that chained PCH files which add threadprivate to an existing var from a previous module include the added attribute.

This should also eliminate the need for these "is this a threadprivate variable" side tables.

lib/CodeGen/CGOpenMPRuntime.cpp
217 ↗(On Diff #14029)

You have the comments backwards here.

219 ↗(On Diff #14029)

Please don't make separate variables called kmpcCtorTy and kmpcCCtorTy. kmpcCopyCtorTy, maybe?

278 ↗(On Diff #14029)

Please document the expectations about this cache variable. It's enough to say that it's a pointer's worth of storage that's reserved for use by the OpenMP runtime.

281 ↗(On Diff #14029)

Why is this lookup by name, rather than by the canonical decl pointer?

288 ↗(On Diff #14029)

Don't create a variable with non-internal linkage and a meaningless name. Two different translation units can use the same meaningless name for the caches for different variables.

If you're going to stick with a meaningless name, you need to use internal linkage for the cache variable.

Alternatively, if you use a meaningful name (perhaps based on the mangled name of the cached variable?), you can potentially share caches across translation units. The optimal sharing strategy depends on the linkage of the original global variable: if it's internal, you should use internal linkage with default visibility (because the cache must not be shared with a variable from a different file), and otherwise you should use linkonce_odr linkage and hidden visibility (so that the cache will be shared as broadly as possible without introducing unnecessary work for the dynamic linker).

310 ↗(On Diff #14029)

Comment your parameters here, please. Especially Addr.

312 ↗(On Diff #14029)

You should pass in the result of isDestructedType() instead of repeatedly recomputing it.

316 ↗(On Diff #14029)

Great place for a comment explaining what each of these functions does. For example, this function re-emits the declaration's initializer into the thread-local copy of the variable.

331 ↗(On Diff #14029)

Please just do a bitcast here.

341 ↗(On Diff #14029)

Document the purpose of this function, please.

lib/CodeGen/CodeGenModule.cpp
1413 ↗(On Diff #14029)

I would go so far as to say that EmitGlobalVarDefinition should implicitly handle the threadprivate stuff itself whenever possible.

3400 ↗(On Diff #14029)

In C++, you can have non-trivial initializers on global variables of arbitrary type, like so:

void *buffer = malloc(1024);

So you may need to PerformInit regardless of whether RD is null.

3401 ↗(On Diff #14029)

Use isDestructedType().

3405 ↗(On Diff #14029)

It is really unfortunate that you need an eager global initializer to register a static local variable. Please change this so that it's only registered the first time that the variable declaration is "evaluated", within the initializer guards.

That should be straightforward: we'll never emit a static local variable before we've fully processed the function body (and hence seen any openmp pragmas lurking within).

lib/CodeGen/ModuleBuilder.cpp
132 ↗(On Diff #14029)

Okay, at the very least, this should be guarded by the OpenMP language option. But I think it would be better to handle this directly in EmitGlobalVarDefinition, when a var is already known to be threadprivate during its emission, which should include all C++ static member cases. It's only true global-scoped variables where CodeGen might process the definition before knowing that it's supposed to be threadprivate.

lib/Serialization/ASTReaderDecl.cpp
2202 ↗(On Diff #14029)

Please follow the pattern of putting each decl kind on its own line.

rjmccall added inline comments.Oct 9 2014, 3:45 PM
lib/CodeGen/CGOpenMPRuntime.cpp
364 ↗(On Diff #14029)

As elsewhere, please rename CCtor to be more easily distinguishable from Ctor.

Please also document what the purpose of this function is and why it's okay to always pass null here.

Hal, John, Thanks for the review!
I'll get back to you soon with the updated patch.

lib/CodeGen/CGOpenMPRuntime.cpp
217 ↗(On Diff #14029)

I'll remove it.

219 ↗(On Diff #14029)

Ok

278 ↗(On Diff #14029)

Ok

281 ↗(On Diff #14029)

VarDecl is the redeclarable construct and it may have several instances. MangledName is the same for all redeclarations. I'll try to improve it.

288 ↗(On Diff #14029)

Agree, my fault. Earlier there was meaningful name, but it was removed somehow during previous updates.

310 ↗(On Diff #14029)

Ok

312 ↗(On Diff #14029)

Agree. I'll rework this

316 ↗(On Diff #14029)

Agree, I'll add comments

331 ↗(On Diff #14029)

Ok

341 ↗(On Diff #14029)

Ok

364 ↗(On Diff #14029)

Ok

lib/CodeGen/CodeGenModule.cpp
1413 ↗(On Diff #14029)

Ok, I'll rework this. I'll add an attribute for thread private variables to handle it in GlobalVarDefinitions.

3400 ↗(On Diff #14029)

I'll fix this

3401 ↗(On Diff #14029)

Ok

3405 ↗(On Diff #14029)

Agree, I'll try to fix this.

lib/CodeGen/ModuleBuilder.cpp
132 ↗(On Diff #14029)

Ok

lib/Serialization/ASTReaderDecl.cpp
2202 ↗(On Diff #14029)

Ok

ABataev updated this revision to Diff 15394.Oct 24 2014, 3:36 AM
ABataev edited edge metadata.

Reworked patch. Added attribute OMPThreadPrivateAttr to simplify codegen. Fixed problem with the codegen for static locals

Looks great! A few comments inline, and then I think it's ready to go.

lib/CodeGen/CGOpenMPRuntime.cpp
374 ↗(On Diff #15394)

Please use

Twine(...) + ".cache."

instead of calling the ctor directly.

421 ↗(On Diff #15394)

Can we avoid having to recompute isConstantInitializer? We should know this from context.

lib/CodeGen/CGOpenMPRuntime.h
170 ↗(On Diff #15394)

Comment: it's a map from the unique names to the variables, not the other way around. Along those same lines, please name it something like InternalVars; the names are the keys, and OMP is implicit from context.

Using a StringMap here isn't the most efficient thing when you're hashing on a place where you have a declaration, but it's okay, and if you think it's worthwhile to reuse the same StringMap for different uses this way, fine. (But please document all the different things that get put in it!)

Instead of holding a naked llvm::GlobalVariable*, though, please use an llvm::AssertingVH<llvm::Constant>, which will correctly be updated if something needs to replaceAllUsesWith the original global variable you created.

lib/Sema/SemaOpenMP.cpp
852 ↗(On Diff #15394)

If you want to get PCH / modules correct when the global decl has been separated from the threadprivate pragma (which might be an unnecessary corner case — up to you), you'll need to add a update record.

The way you do that is to:

  1. check to see if the decl you're modifying came from an AST file, and if so:
  2. add a new virtual method to ASTMutationListener,
  3. implement it in ASTWriter by adding an update record to the AST, and then
  4. handle that update record in ASTReader by adding the attribute retroactively (to all subsequent redeclarations).
ABataev updated this revision to Diff 15841.Nov 6 2014, 12:35 AM

Updated patch after review.

You never actually call DeclarationMarkedOpenMPThreadPrivate in the first place. The right place to do that is at the point when you add the attribute.

You also need to test that this works. The basic idea is that the first PCH file should declare the variable, the second should declare it to be thread-private, and the third uses it (in some way that verifies that it was marked thread-private; preferably using a Sema check, but an IRGen test is also okay). You can look at test/PCH/chain-categories.m and test/PCH/chain-implicit-definition.cpp for examples of how to do that. (The test should go in test/PCH, not test/OpenMP.)

ABataev updated this revision to Diff 15967.Nov 9 2014, 11:48 PM

Fix after review.

Awesome, thanks. Looks great.

ABataev closed this revision.Nov 10 2014, 7:57 PM
ABataev updated this revision to Diff 16023.

Closed by commit rL221663 (authored by @ABataev).