[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:
> 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) {...}
```


================
Comment at: clang/test/OpenMP/irbuilder_simd.cpp:72
+// CHECK-NEXT: ![[META9]]  = distinct !{![[META9]], ![[META10:[0-9]+]], ![[META6]]}
+// CHECK-NEXT: ![[META10]]  = !{!"llvm.loop.parallel_accesses", ![[META8]]}
----------------
nit: maybe add newline


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1773
   OMPBuilder.applySimd(DL, CLI);
+  OMPBuilder.applySimdlen(DL, CLI, ConstantInt::get(Type::getInt32Ty(Ctx), 3));
 
----------------
Can we please add this as a new test instead of modifying the existing one?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129149