Diff Detail
Event Timeline
lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
29 | Please document the meaning of this string. | |
lib/CodeGen/CGOpenMPRuntime.h | ||
46 | This should have some overall comment explaining what this is the location of. | |
49 | c-style? | |
73 | This needs to be documented (including the fields too). | |
81 | What's this? | |
lib/Sema/SemaOpenMP.cpp | ||
675 | Could these names conflict with user-provided names? (Should they be in the implementation namespace?) |
lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
29 | Ok. | |
lib/CodeGen/CGOpenMPRuntime.h | ||
46 | Ok, I'll add comment | |
49 | Yes, all these comments are taken from http://llvm.org/svn/llvm-project/openmp/trunk/runtime/src/kmp.h | |
73 | Ok, will be commented. | |
81 | Default position used for initialization of all ident_t objects. I'll described it. | |
lib/Sema/SemaOpenMP.cpp | ||
675 | Yes, that's possible. I'll rename them. |
- Original Message -----
From: "Alexey Bataev" <a.bataev@hotmail.com>
To: dgregor@apple.com, gribozavr@gmail.com, hfinkel@anl.gov, fraggamuffin@gmail.com, cbergstrom@pathscale.com,
richard@metafoo.co.uk, "a bataev" <a.bataev@hotmail.com>
Cc: cfe-commits@cs.uiuc.edu
Sent: Friday, March 7, 2014 5:56:04 AM
Subject: Re: [PATCH] [OPENMP] Initial codegen for '#pragma omp parallel'Comment at: lib/CodeGen/CGOpenMPRuntime.h:45
@@ +44,3 @@
+public:
+ enum OpenMPLocationFlags {+ /// \brief Use tramponline for internal microtask.
hfinkel@anl.gov wrote:
This should have some overall comment explaining what this is the
location of.Ok, I'll add comment
Comment at: lib/CodeGen/CGOpenMPRuntime.h:48
@@ +47,3 @@
+ OMP_IDENT_IMD = 0x01,
+ /// \brief Use c-stile ident structure.+ OMP_IDENT_KMPC = 0x02,
hfinkel@anl.gov wrote:
c-style?
Yes, all these comments are taken from
http://llvm.org/svn/llvm-project/openmp/trunk/runtime/src/kmp.h
I was wondering whether 'c-stile' was a typo, and should have been c-style?
-Hal
Comment at: lib/CodeGen/CGOpenMPRuntime.h:72
@@ +71,3 @@
+private:
+ enum Ident_TFields {+ Ident_T_Reserved_1,
hfinkel@anl.gov wrote:
This needs to be documented (including the fields too).
Ok, will be commented.
Comment at: lib/CodeGen/CGOpenMPRuntime.h:80
@@ +79,3 @@
+ CodeGenModule &CGM;
+ llvm::Constant *DefaultOpenMPPSource;+ llvm::StructType *Ident_TTy;
hfinkel@anl.gov wrote:
What's this?
Default position used for initialization of all ident_t objects. I'll
described it.Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:28
@@ +27,3 @@
+CGOpenMPRuntime::CGOpenMPRuntime(CodeGenModule &CGM) : CGM(CGM) {
+ DefaultOpenMPPSource =
CGM.GetAddrOfConstantCString(";unknown;unknown;0;0;;");+ DefaultOpenMPPSource =
hfinkel@anl.gov wrote:
Please document the meaning of this string.
Ok.
Comment at: lib/Sema/SemaOpenMP.cpp:675
@@ +674,3 @@
+ { std::make_pair("global_tid", KmpInt32PtrTy),
+ std::make_pair("bound_tid", KmpInt32PtrTy),
+ std::make_pair(StringRef(), QualType()) // __context withshared vars
hfinkel@anl.gov wrote:
Could these names conflict with user-provided names? (Should they
be in the implementation namespace?)Yes, that's possible. I'll rename them.
I think that this generally looks good. I still have a concern about the names: we generate names like context. Now I understand that a user should not generate names like this, but what if they do? Does it crash? We need to make sure that have reasonable error detection and reporting for these things. In the clang-omp branch the names are generated with a preceding ''.' -- ".kmpc_global_thread_num." for example -- and this can never conflict with a user-provided identifier. This seems safer, so why are you not doing that here?
I agree that this is looking really good.
lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
54–69 | Looks like this can be replaced by IRBuilder::InsertPointGuard? | |
175 | We put the break; inside the braces. | |
lib/CodeGen/CGOpenMPRuntime.h | ||
50–51 | Typo of 'trampoline'. | |
50–65 | Our coding convention doesn't use ALL_CAPS here. These should probably be OLF_IdentImd, OLF_IdentKmpc, ... to follow that convention (though I'm not really sure what these names mean, and they don't seem to directly correspond to the comments. What does IMD mean, or KMPC, for instance?) Alternatively, you could explicitly state that these are named to match the names from kmp.h, rather than just saying the descriptions are from there. | |
lib/CodeGen/CGStmtOpenMP.cpp | ||
32–36 | Why are you creating the CGCapturedStmtInfo on the heap? Seems weird, since it looks like you delete it two lines afterwards. | |
lib/Sema/SemaOpenMP.cpp | ||
673–680 | Formatting here doesn't match our usual conventions. For the braced initialization, put the { on the previous line, dedent the };. Dedent the } for the case, and move the break; before it. | |
685–686 | Yuck. NUM_OPENMP_DIRECTIVES shouldn't be a member of the enum, to remove the need to do this. | |
lib/Sema/TreeTransform.h | ||
9784 | Capital I please. | |
lib/Serialization/ASTReaderDecl.cpp | ||
1080–1088 | Do you have test coverage for this? |
Richard, thanks for the review!!! I do appreciate it.
lib/CodeGen/CGOpenMPRuntime.cpp | ||
---|---|---|
54–69 | Yes, thanks, missed this one! | |
175 | Thanks, will be fixed. | |
lib/CodeGen/CGOpenMPRuntime.h | ||
50–51 | Will be fixed | |
50–65 | Richard, ok, I'll add required comment | |
lib/CodeGen/CGStmtOpenMP.cpp | ||
32–36 | I just followed the standard pattern for CapturedStmt (see CGStmt.cpp, CodeGenFunction::EmitCapturedStmt). But I'll rework it. | |
lib/Sema/SemaOpenMP.cpp | ||
673–680 | Ok. I'll reformat it. | |
685–686 | This one is needed for checking that the directive is correctly specified. | |
lib/Sema/TreeTransform.h | ||
9784 | Ok | |
lib/Serialization/ASTReaderDecl.cpp | ||
1080–1088 | Yes, parallel_codegen.cpp and parallel_ast_print.cpp. |
LGTM, but this is a significant enough change to CodeGen that I'd like rjmccall to look at it too.
lib/Serialization/ASTReaderDecl.cpp | ||
---|---|---|
1080–1088 | Do those really test the serialization/deserialization code? |
lib/Serialization/ASTReaderDecl.cpp | ||
---|---|---|
1080–1088 | Yes, take a look at these tests. There are 3 passes: 1) initial test "as is"; 2) "-emit-pch" pass; 3) "-include-pch" pass. The 2-nd and the 3-rd passes check serialization/deserialization of AST nodes. |
lib/CodeGen/CodeGenModule.h | ||
---|---|---|
258 | Probably not a comment for your line, but it'd be safer to use std::unique_ptr instead of raw pointers. That would also eliminate explicit calls to delete. |
include/clang/Sema/Sema.h | ||
---|---|---|
2984 | This typedef has a very generic-sounding name and a very specific purpose. | |
lib/CodeGen/CGOpenMPRuntime.cpp | ||
32 | Please create this string lazily. | |
48 | Please create this global lazily. | |
49 | Please /*comment*/ arguments like "true". | |
54 | I would not expect this method to cause code to be emitted at the current insertion point. Maybe call it emitUpdateLocation or something like that? | |
63 | Remove this entire thing and use an llvm::Twine inline in the argument to CreateTempAlloca. | |
92 | This check is always true; you bailed out earlier if it wasn't. | |
126 | I'm not sure why you're using an alloca if you're going to unconditionally do the call in a place that dominates the entire function. Just do the call and use the result wherever you need it. | |
lib/CodeGen/CGOpenMPRuntime.h | ||
79 | The _T suffix makes this pretty confusing to read — it's ident_t in the original, presumably, and capitalizing it does it no favors. I would rename: Ident_TTy -> IdentTy Ident_TFields -> IdentFieldIndex Ident_T_* -> IdentField_* Also, put this enum and its comment next to the declaration of IdentTy, and include a C-style struct definition in the comment. | |
98 | Please add a comment to the end of the line describing this function type in C terms, if reasonable. | |
101 | Does this need to be as specific as an AllocaInst? Also, these maps seem to contain information that's really specific to a CodeGenFunction; you should get notified when a CGF is torn down so that you can remove entries as appropriate. | |
lib/CodeGen/CGStmtOpenMP.cpp | ||
2 | There's no reason to make this a separate file. Just keep all the OpenMP code in one place as much as possible. | |
33 | You should declare CGF inside this block, too. | |
43 | Please comment this magic number. | |
lib/Sema/SemaOpenMP.cpp | ||
685 | Please define your enums so that things like this case aren't necessary. If you really need a NUM_OPENMP_DIRECTIVES constant, you can define it right after the enum. | |
745 | This comment is useless. Please instead cite *why* you can mark the captured decl as no throw. |
John,
Thanks for review! See my comments. Updated patch will be prepared soon.
Best regards,
Alexey Bataev
Software Engineer
Intel Compiler Team
Intel Corp.
include/clang/Sema/Sema.h | ||
---|---|---|
2984 | Ok, I'll rename it. | |
lib/CodeGen/CGOpenMPRuntime.cpp | ||
32 | Ok | |
48 | Ok | |
49 | Ok | |
54 | Ok, I'll rename it. | |
63 | Ok | |
92 | Agree, I'll remove it | |
126 | Agree | |
lib/CodeGen/CGOpenMPRuntime.h | ||
79 | Ok | |
98 | Ok | |
101 | I'll try to rework it. | |
lib/CodeGen/CGStmtOpenMP.cpp | ||
2 | I think it would be better to keep a separate file because there should be a lot of code for OpenMP. | |
33 | Agree | |
43 | Ok | |
lib/Sema/SemaOpenMP.cpp | ||
685 | Ok | |
745 | Will be removed |
This typedef has a very generic-sounding name and a very specific purpose.