This is an archive of the discontinued LLVM Phabricator instance.

[OPENMP] Initial codegen for '#pragma omp parallel'
ClosedPublic

Authored by ABataev on Feb 25 2014, 10:26 PM.

Diff Detail

Event Timeline

hfinkel added inline comments.Mar 6 2014, 9:25 PM
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?)

ABataev added inline comments.Mar 7 2014, 3:56 AM
lib/CodeGen/CGOpenMPRuntime.cpp
29

Ok.

lib/CodeGen/CGOpenMPRuntime.h
46

Ok, I'll add comment

49
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.

ABataev updated this revision to Unknown Object (????).Mar 7 2014, 4:09 AM

Fix after Hal's review

  • 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 with

shared 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.

http://llvm-reviews.chandlerc.com/D2883

Oh, yes, of course "style". I'll fix that

ABataev updated this revision to Unknown Object (????).Mar 10 2014, 8:02 PM

Fixed typo in comment

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?

ABataev updated this revision to Unknown Object (????).Mar 30 2014, 8:35 PM

Update after latest Hal's review

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.

ABataev updated this revision to Unknown Object (????).Apr 10 2014, 9:13 PM

Rework after Richard's review

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?

ABataev added inline comments.Apr 13 2014, 7:34 PM
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.

zinovy.nis added inline comments.Apr 17 2014, 3:43 AM
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.

rjmccall added inline comments.Apr 21 2014, 2:55 PM
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

ABataev updated this revision to Unknown Object (????).Apr 22 2014, 4:36 AM

Update after John McCall review

ABataev accepted this revision.May 6 2014, 2:26 AM
ABataev added a reviewer: ABataev.

LGTM (C. Bergström, Mon, 5 May 2014 23:48:34)

This revision is now accepted and ready to land.May 6 2014, 2:26 AM
ABataev closed this revision.May 6 2014, 3:16 AM

Committed revision 208077.