[PATCH] D15321: [OpenMP 4.0]Parsing and Sema support for 'omp declare target' directive (accelerator support)
Alexey Bataev via cfe-commits
cfe-commits at lists.llvm.org
Mon Dec 7 22:48:55 PST 2015
ABataev added a comment.
I think we'd better to try to use attributes for declare target. I think it will be much easier and correct solution rather than adding a new declaration
================
Comment at: include/clang/AST/DeclBase.h:164
@@ -163,3 +163,3 @@
/// has been declared outside any function.
- IDNS_LocalExtern = 0x0800
+ IDNS_LocalExtern = 0x0800,
};
----------------
I don't think this is required, revert it back please
================
Comment at: include/clang/AST/DeclOpenMP.h:18-21
@@ -17,4 +17,6 @@
-#include "clang/AST/DeclBase.h"
+#include "clang/AST/Decl.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/Type.h"
#include "llvm/ADT/ArrayRef.h"
----------------
Again, I don't think this change is required. You don't use neither Exprs, nor Types in your changes below
================
Comment at: include/clang/AST/DeclOpenMP.h:98
@@ +97,3 @@
+///
+class OMPDeclareTargetDecl : public Decl, public DeclContext {
+ friend class ASTDeclReader;
----------------
I'm not sure that we need to add this kind of declaration. Most probably it is enough just to have an attribute
================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:7751
@@ -7745,1 +7750,3 @@
+ "declaration is not declared in any declare target region">,
+ InGroup<OpenMPClauses>;
def err_omp_aligned_expected_array_or_ptr : Error<
----------------
I don't think that OpenMPClauses is suitable warning group for this warning. Maybe you need to add a new one, for example, target specific
================
Comment at: include/clang/Basic/OpenMPKinds.h:30
@@ -30,1 +29,3 @@
+ OMPD_unknown,
+ OMPD_empty
};
----------------
OMPD_empty is not required, this must be deleted
================
Comment at: lib/AST/Decl.cpp:21
@@ -20,2 +20,3 @@
#include "clang/AST/DeclObjC.h"
+#include "clang/AST/DeclOpenMP.h"
#include "clang/AST/DeclTemplate.h"
----------------
Do you really need this header file here?
================
Comment at: lib/AST/DeclBase.cpp:653
@@ -652,2 +652,3 @@
case OMPThreadPrivate:
+ case OMPDeclareTarget:
case Empty:
----------------
The change is not properly formatted
================
Comment at: lib/AST/DeclOpenMP.cpp:10
@@ -9,3 +9,3 @@
/// \file
-/// \brief This file implements OMPThreadPrivateDecl class.
+/// \brief This file implements OMPThreadPrivateDecl. OMPDeclareTarget class.
///
----------------
comma?
================
Comment at: lib/AST/DeclPrinter.cpp:95
@@ -94,2 +94,3 @@
void VisitOMPThreadPrivateDecl(OMPThreadPrivateDecl *D);
+ void VisitOMPDeclareTargetDecl(OMPDeclareTargetDecl *D);
----------------
Not formatted
================
Comment at: lib/AST/ItaniumMangle.cpp:23
@@ -22,2 +22,3 @@
#include "clang/AST/DeclObjC.h"
+#include "clang/AST/DeclOpenMP.h"
#include "clang/AST/DeclTemplate.h"
----------------
I don't think this header file is required
================
Comment at: lib/AST/MicrosoftMangle.cpp:22
@@ -21,2 +21,3 @@
#include "clang/AST/DeclObjC.h"
+#include "clang/AST/DeclOpenMP.h"
#include "clang/AST/DeclTemplate.h"
----------------
Again, Are you sure this is required?
================
Comment at: lib/CodeGen/CGDecl.cpp:1873-1874
@@ +1872,4 @@
+ // that is a valid region for a target
+ OpenMPSupport.startOpenMPRegion(false);
+ OpenMPSupport.setTargetDeclare(true);
+
----------------
There is no such object
================
Comment at: lib/CodeGen/CodeGenModule.h:1067-1102
@@ -1065,1 +1066,38 @@
+ class OpenMPSupportStackTy {
+ /// \brief A set of OpenMP threadprivate variables.
+ llvm::DenseMap<const Decl *, const Expr *> OpenMPThreadPrivate;
+ /// \brief A set of OpenMP private variables.
+ typedef llvm::DenseMap<const Decl *, llvm::Value *> OMPPrivateVarsTy;
+ struct OMPStackElemTy {
+ OMPPrivateVarsTy PrivateVars;
+ llvm::BasicBlock *IfEnd;
+ Expr *IfClauseCondition;
+ llvm::SmallVector<llvm::Value *, 16> offloadingMapArguments;
+ llvm::Function *ReductionFunc;
+ CodeGenModule &CGM;
+ CodeGenFunction *RedCGF;
+ llvm::SmallVector<llvm::Type *, 16> ReductionTypes;
+ llvm::DenseMap<const VarDecl *, unsigned> ReductionMap;
+ llvm::StructType *ReductionRec;
+ llvm::Value *ReductionRecVar;
+ llvm::Value *RedArg1;
+ llvm::Value *RedArg2;
+ llvm::Value *ReduceSwitch;
+ llvm::BasicBlock *BB1;
+ llvm::Instruction *BB1IP;
+ llvm::BasicBlock *BB2;
+ llvm::Instruction *BB2IP;
+ llvm::Value *LockVar;
+ llvm::BasicBlock *LastprivateBB;
+ llvm::Instruction *LastprivateIP;
+ llvm::BasicBlock *LastprivateEndBB;
+ llvm::Value *LastIterVar;
+ llvm::Value *TaskFlags;
+ llvm::Value *PTaskTValue;
+ llvm::Value *PTask;
+ llvm::Value *UntiedPartIdAddr;
+ unsigned UntiedCounter;
+ llvm::Value *UntiedSwitch;
+ llvm::BasicBlock *UntiedEnd;
+ CodeGenFunction *ParentCGF;
----------------
No, this must be deleted. We don't use it here
================
Comment at: lib/Parse/ParseDecl.cpp:3624
@@ -3623,3 +3623,3 @@
// Silence possible warnings.
- (void)Res;
+ (void)Res;
continue;
----------------
Restore original line
================
Comment at: lib/Parse/ParseOpenMP.cpp:36-39
@@ +35,6 @@
+ OMPD_cancellation_point}, //0
+ {OMPD_unknown /*declare*/, OMPD_target /*target*/,
+ OMPD_declare_target }, //1
+ {OMPD_unknown /*end*/, OMPD_unknown /*declare*/,
+ OMPD_end_declare_target }, //2
+ {OMPD_target, OMPD_unknown /*data*/, OMPD_target_data}, //3
----------------
Formatting broken
================
Comment at: lib/Parse/ParseOpenMP.cpp:58-63
@@ -54,1 +57,8 @@
+ !P.getPreprocessor().getSpelling(Tok).compare("cancellation") ||
+ ((i == 1 || i == 3) &&
+ !P.getPreprocessor().getSpelling(Tok).compare("declare")) ||
+ ((i == 2) &&
+ !P.getPreprocessor().getSpelling(Tok).compare("end")) ||
+ ((i == 3) &&
+ !P.getPreprocessor().getSpelling(Tok).compare("target"));
} else {
----------------
Formatting
================
Comment at: lib/Parse/ParseOpenMP.cpp:80-83
@@ -69,3 +79,6 @@
!P.getPreprocessor().getSpelling(Tok).compare("point")) ||
- ((i == 1) && !P.getPreprocessor().getSpelling(Tok).compare("data"));
+ ((i == 2) &&
+ !P.getPreprocessor().getSpelling(Tok).compare("declare")) ||
+ ((i == 3) &&
+ !P.getPreprocessor().getSpelling(Tok).compare("data"));
} else {
----------------
Formatting
================
Comment at: lib/Parse/ParseOpenMP.cpp:209
@@ -142,2 +208,3 @@
case OMPD_taskloop_simd:
+ default:
Diag(Tok, diag::err_omp_unexpected_directive)
----------------
Do not add default:, coding standard recommends to not use it
================
Comment at: lib/Parse/ParseOpenMP.cpp:387-392
@@ -318,2 +386,8 @@
break;
+ default:
+ Diag(Tok, diag::err_omp_unexpected_directive)
+ << getOpenMPDirectiveName(DKind);
+ while (!SkipUntil(tok::annot_pragma_openmp_end))
+ ;
+ break;
}
----------------
Do not add it
================
Comment at: lib/Parse/ParseOpenMP.cpp:394
@@ -319,2 +393,3 @@
}
+
return Directive;
----------------
Restore original
================
Comment at: lib/Sema/SemaExpr.cpp:13566-13568
@@ -13565,1 +13565,5 @@
Decl *D, Expr *E, bool OdrUse) {
+ if (SemaRef.IsDeclContextInOpenMPTarget(SemaRef.CurContext)) {
+ SemaRef.CheckDeclIsAllowedInOpenMPTarget(E, D);
+ }
+
----------------
Formatting. Remove braces
================
Comment at: lib/Sema/SemaOpenMP.cpp:1139-1140
@@ -1105,3 +1138,4 @@
QualType ExprType = VD->getType().getNonReferenceType();
- ExprResult DE = buildDeclRefExpr(*this, VD, ExprType, Id.getLoc());
+ //ExprResult DE = buildDeclRefExpr(*this, VD, ExprType, Id.getLoc());
+ ExprResult DE = BuildDeclRefExpr(VD, ExprType, VK_LValue, Id.getLoc());
return DE;
----------------
I don't understand why you changed this.
================
Comment at: lib/Sema/SemaOpenMP.cpp:1414-1415
@@ +1413,4 @@
+
+static bool IsCXXRecordForMappable(Sema &SemaRef, SourceLocation Loc,
+ DSAStackTy *Stack, CXXRecordDecl *RD);
+
----------------
Why you need this declaration if earlier you have a definition?
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:353
@@ -352,2 +352,3 @@
void VisitOMPThreadPrivateDecl(OMPThreadPrivateDecl *D);
+ void VisitOMPDeclareTargetDecl(OMPDeclareTargetDecl *D);
----------------
Formatting
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:2411
@@ -2406,1 +2410,3 @@
+ isa<OMPThreadPrivateDecl>(D) ||
+ isa<OMPDeclareTargetDecl>(D) )
return true;
----------------
Formatting
================
Comment at: lib/Serialization/ASTReaderDecl.cpp:3307-3309
@@ -3300,2 +3306,5 @@
break;
+ case DECL_OMP_DECLARETARGET:
+ D = OMPDeclareTargetDecl::CreateDeserialized(Context, ID);
+ break;
case DECL_EMPTY:
----------------
Formatting
================
Comment at: lib/Serialization/ASTWriterDecl.cpp:134
@@ -133,2 +133,3 @@
void VisitOMPThreadPrivateDecl(OMPThreadPrivateDecl *D);
+ void VisitOMPDeclareTargetDecl(OMPDeclareTargetDecl *D);
----------------
Formatting
Repository:
rL LLVM
http://reviews.llvm.org/D15321
More information about the cfe-commits
mailing list