psoni2628 added inline comments.
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2596
// Check for unsupported clauses
- if (!S.clauses().empty()) {
- // Currently no clause is supported
- return false;
+ for (OMPClause *C : S.clauses()) {
+ // Currently only simdlen clause is supported
----------------
shraiysh wrote:
> psoni2628 wrote:
> > psoni2628 wrote:
> > > arnamoy10 wrote:
> > > > I am just wondering whether we should have a check to make sure that we
> > > > are processing the clauses of only `simd` directive here. Because the
> > > > function takes a general `OMPExecutableDirective` as argument
> > > That's a fair point. I guess `isSupportedByOpenMPIRBuilder` could be used
> > > for other directive types other than simd, even though it's not right now.
> > Would it make more sense to only guard the checking of clauses with a check
> > for `OMPSimdDirective`, or the whole thing? I believe even the code below,
> > which checks for an ordered directive, is also specifically for `simd`?
> >
> >
> > Example of guarding the whole thing:
> >
> > ```
> > if(dyn_cast<OMPSimdDirective>(S)) {
> > // Check for unsupported clauses
> > for (OMPClause *C : S.clauses()) {
> > // Currently only simdlen clause is supported
> > if (dyn_cast<OMPSimdlenClause>(C))
> > continue;
> > else
> > return false;
> > }
> >
> > // Check if we have a statement with the ordered directive.
> > // Visit the statement hierarchy to find a compound statement
> > // with a ordered directive in it.
> > if (const auto *CanonLoop = dyn_cast<OMPCanonicalLoop>(S.getRawStmt()))
> > {
> > if (const Stmt *SyntacticalLoop = CanonLoop->getLoopStmt()) {
> > for (const Stmt *SubStmt : SyntacticalLoop->children()) {
> > if (!SubStmt)
> > continue;
> > if (const CompoundStmt *CS = dyn_cast<CompoundStmt>(SubStmt)) {
> > for (const Stmt *CSSubStmt : CS->children()) {
> > if (!CSSubStmt)
> > continue;
> > if (isa<OMPOrderedDirective>(CSSubStmt)) {
> > return false;
> > }
> > }
> > }
> > }
> > }
> > }
> > }
> > ```
> Can we instead have separate `isSupportedByOpenMPIRBuilder` for every
> directive to avoid bloating the function with checks and if conditions?
> ```
> static bool isSupportedByOpenMPIRBuilder(const OMPSimdDirective &S) {...}
> void CodeGenFunction::EmitOMPSimdDirective(const OMPSimdDirective &S) {...}
>
> static bool isSupportedByOpenMPIRBuilder(const OMPOrderedDirective &S) {...}
> void CodeGenFunction::EmitOMPOrderedDirective(const OMPOrderedDirective &S)
> {...}
>
> static bool isSupportedByOpenMPIRBuilder(const OMPTaskDirective &S) {...}
> void CodeGenFunction::EmitOMPOrderedDirective(const OMPTaskDirective &S) {...}
> ```
It was decided in D114379 to use `OMPExecutableDirective` in order to allow
this function to be reused for constructs such as `for simd`. Do you wish to
undo this now, and instead specialize the function?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129149/new/
https://reviews.llvm.org/D129149
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
