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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D129149#3633627 <https://reviews.llvm.org/D129149#3633627>, @Meinersbur wrote:

> Why not have `simdlen` an argument to `applySimd` (which can be null if not used)? In this case it happens to be just adding some metadata, but for others such as `unrollLoopPartial` it does not make sense to have the unroll factor set by a different method. It would also allow better consistency checks if passed as a parameter.

Sure.



================
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
----------------
Meinersbur wrote:
> psoni2628 wrote:
> > shraiysh wrote:
> > > 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?
> > I agree with you that having a general function for `OMPExecutableDirective` will probably result in a very large, bloated function. I have tentatively just specialized the function for `OMPSimdDirective`.
> A function taking only `OMPSimdDirective` will still not handle `OMPForSimdDirective` and the code for handling it would need to be duplicated for both (and `OMPDistributeSimdDirective`, and `OMPDistributeParallelForSimdDirective`, ...)
> 
> I'd expect the check the most common to be the (non-)existence of a clause such as `simdlen`. Once support for it is added, it should be supported for all combined/composite constructs. Code Duplication not necessary. We do not need to check whether `OMPExecutableDirective` is is a simd construct because Sema will reject `simdlen` on any other construct anyway. 
> 
> Therefore is still favor taking a `OMPExecutableDirective` argument. OpenMPIRBuilder does not support combined/composite construct yet, but whoever adds it has to face the immense problem of refactoring all the `isSupportedByOpenMPIRBuilder` implementations to have to rewrite that function for all combined/composite constructs. Better consider the possibility of combined/composite constructs from the beginning.
Unsure why this has to go into another round of comments:

This is, as I noted, a mood discussion. If this passes all tests there is no good reason to make it `OMPExecutableDirective` right now. As we add anything that does not work with this API we need to make a choice and include changes anyway. Arguing the type of the argument right now, = the ~15 letter difference, is making anything harder in the future, is arguably not true. As always, designing something without coverage and use case at hand is discouraged, especially if the design does not exclude an extension.



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

https://reviews.llvm.org/D129149