[PATCH] D129149: [OMPIRBuilder] Add support for simdlen clause

[Shraiysh via Phabricator via llvm-commits]

shraiysh 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
----------------
psoni2628 wrote:
> 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?
I wasn't aware of the earlier discussion about this. However it seems like it was suggested to try either splitting the function or merging it with slight preference for merging (I didn't understand how that would help constructs like `for simd` because I haven't worked with `for simd` much and will take others' word for it).

The suggested code change in previous reply - checking of clauses with a check for OMPSimdDirective - this seems like it would eventually become a huge function because of such checks. 

@Meinersbur it would be great if you could please advise on this - if keeping the checks for all executable directives in the same function will be helpful in the long run - allowing us to reuse checks or should we split it? A third alternative would be to have both functions, one specialized for SIMD and one for ExecutableConstruct, where the latter might handle combined constructs like `for simd` etc. Would that arrangement work?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129149/new/

https://reviews.llvm.org/D129149