This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Improve mappable expressions Sema.
ClosedPublic

Authored by sfantao on Apr 21 2016, 11:55 AM.

Details

Summary

This patch adds logic to save the components of mappable expressions in the clause that uses it, so that they don't have to be recomputed during codegen. Given that the mappable components are (will be) used in several clauses a new geneneric implementation OMPMappableExprListClause is used that extends the existing OMPVarListClause.

This patch does not add new tests. The goal is to preserve the existing functionality while storing more info in the clauses.

Diff Detail

Event Timeline

sfantao updated this revision to Diff 54553.Apr 21 2016, 11:55 AM
sfantao retitled this revision from to [OpenMP] Improve mappable expressions Sema..
sfantao updated this object.
sfantao added subscribers: caomhin, cfe-commits.
ABataev added inline comments.Apr 21 2016, 11:24 PM
include/clang/AST/OpenMPClause.h
2789–2797

Please, use in-class initialization of class members, like:

Expr *AssociatedExpression = nullptr;

Also, mark this constructor as explicit

2859

I'm not sure exactly, but ArrayRef<> adds const modifier automatically, so you can use just ValueDecl*, without const, as template specialization parameter

3069–3072

Again, no const

3092

const

3110

const

3135

remove this->

3150

remove this->

3160

remove this->

3175

remove this->

3178

remove this->

3206

const

3232

const

lib/AST/OpenMPClause.cpp
546–547

I don't see where you modifies Cache set, so I presume this won't work at all.

lib/Sema/SemaOpenMP.cpp
345–348

Maybe it can be converted to:

bool checkMappableExprComponentListsForDecl(ValueDecl *VD, bool CurrentRegionOnly,
                                         const llvm::function_ref<bool(OMPClauseMappableExprCommon::MappableExprComponentListRef)> &Check)
lib/Serialization/ASTReaderStmt.cpp
1878

break must be inside braces

sfantao updated this revision to Diff 54672.Apr 22 2016, 9:53 AM
sfantao marked 15 inline comments as done.
  • Address review comments. Fix bug in the evaluation of the unique declarations.

Hi Alexey,

Thanks for the review!

include/clang/AST/OpenMPClause.h
2789–2797

Done!

2859

I am doing as you say.

3135

We have to use this because I is member of a template parent class and cannot be resolved by the template class definition. It can only be resolved by accessing the specific template instance pointed to by this.

3150

Have to keep this for the reason I mentioned above.

3160

Have to keep this for the reason I mentioned above.

3175

Have to keep this for the reason I mentioned above.

3178

Have to keep this for the reason I mentioned above.

lib/AST/OpenMPClause.cpp
546–547

Thanks for catching that! You are right. I fixed that. This was not causing bad behavior, other than allocating more memory than what is actually required in the trailing objects.

lib/Sema/SemaOpenMP.cpp
345–348

Done!

ABataev added inline comments.Apr 25 2016, 4:44 AM
lib/AST/OpenMPClause.cpp
546

I think you'd better to count and to store canonical decls rather than the decls themselves. It will resolves issues for decls, where one decl is declared as an alias to another decl

sfantao updated this revision to Diff 54941.Apr 25 2016, 4:24 PM
  • Use canonical declarations associated with mappable expressions.
sfantao marked an inline comment as done.Apr 25 2016, 4:26 PM
sfantao added inline comments.
lib/AST/OpenMPClause.cpp
546

Ok, using canonical declarations now.

ABataev accepted this revision.Apr 25 2016, 10:26 PM
ABataev edited edge metadata.
ABataev added inline comments.
lib/AST/OpenMPClause.cpp
546

No, I mean you'd better to check and store canonical decls in Cache, like:

if (Cache.count(D->getCaonicalDecl())
Cache.insert(D->getCanonicalDecl());
This revision is now accepted and ready to land.Apr 25 2016, 10:26 PM
sfantao marked 2 inline comments as done.Apr 26 2016, 7:23 AM

Hi Alexey,

Thanks for the review.

lib/AST/OpenMPClause.cpp
546

Oh, okay, I am doing as you say.

sfantao updated this revision to Diff 55002.Apr 26 2016, 7:24 AM
sfantao marked an inline comment as done.
sfantao edited edge metadata.
  • Explicitelly store canonical declaration when looking for unique declarations.
sfantao closed this revision.Apr 26 2016, 8:00 AM