Details
Diff Detail
Event Timeline
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.
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.
These changes look good to me, but I am new to clang. I did not attempt a build with these changes applied.
Builds fine. Change looks OK, but as this is codegen, will need more
established eyes like Hal.
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
It's a bit sad this community cares so little when there's good code to be merged.
lib/CodeGen/CGDeclCXX.cpp | ||
---|---|---|
547 | Like John said for EmitOMPSerialCall, this seems like it belongs in CGOpenMPRuntime.cpp. | |
620 | Why not use the SourceLocation of the variable's initializer? | |
lib/Parse/Parser.cpp | ||
627 | Is this a separate change? | |
lib/Serialization/ASTReaderDecl.cpp | ||
2186 | This is a separate change. |
Ping
lib/CodeGen/CGDeclCXX.cpp | ||
---|---|---|
547 | I'll rework this to remove direct calls to runtime functions. | |
620 | Will be fixed. | |
lib/Parse/Parser.cpp | ||
627 | Nope, this is required for codegen, without it the declaration is not passed to codegen consumer. | |
lib/Serialization/ASTReaderDecl.cpp | ||
2186 | Nope, it's a part of codegen, without this change codegen for construct loaded from .pch does not work. |
- 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.
LGTM.
lib/CodeGen/CodeGenModule.cpp | ||
---|---|---|
1390 | 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). |
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 | ||
---|---|---|
199 | You have the comments backwards here. | |
201 | Please don't make separate variables called kmpcCtorTy and kmpcCCtorTy. kmpcCopyCtorTy, maybe? | |
260 | 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. | |
263 | Why is this lookup by name, rather than by the canonical decl pointer? | |
270 | 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). | |
292 | Comment your parameters here, please. Especially Addr. | |
294 | You should pass in the result of isDestructedType() instead of repeatedly recomputing it. | |
298 | 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. | |
313 | Please just do a bitcast here. | |
323 | Document the purpose of this function, please. | |
lib/CodeGen/CodeGenModule.cpp | ||
1390 | I would go so far as to say that EmitGlobalVarDefinition should implicitly handle the threadprivate stuff itself whenever possible. | |
3348 | 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. | |
3349 | Use isDestructedType(). | |
3353 | 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 | ||
108 | 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 | ||
2186 | Please follow the pattern of putting each decl kind on its own line. |
lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
346 | 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 | ||
---|---|---|
199 | I'll remove it. | |
201 | Ok | |
260 | Ok | |
263 | VarDecl is the redeclarable construct and it may have several instances. MangledName is the same for all redeclarations. I'll try to improve it. | |
270 | Agree, my fault. Earlier there was meaningful name, but it was removed somehow during previous updates. | |
292 | Ok | |
294 | Agree. I'll rework this | |
298 | Agree, I'll add comments | |
313 | Ok | |
323 | Ok | |
346 | Ok | |
lib/CodeGen/CodeGenModule.cpp | ||
1390 | Ok, I'll rework this. I'll add an attribute for thread private variables to handle it in GlobalVarDefinitions. | |
3348 | I'll fix this | |
3349 | Ok | |
3353 | Agree, I'll try to fix this. | |
lib/CodeGen/ModuleBuilder.cpp | ||
108 | Ok | |
lib/Serialization/ASTReaderDecl.cpp | ||
2186 | Ok |
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 | ||
---|---|---|
222 | Please use Twine(...) + ".cache." instead of calling the ctor directly. | |
269 | Can we avoid having to recompute isConstantInitializer? We should know this from context. | |
lib/CodeGen/CGOpenMPRuntime.h | ||
168 | 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:
|
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.)
Like John said for EmitOMPSerialCall, this seems like it belongs in CGOpenMPRuntime.cpp.