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:
> > 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`.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129149/new/
https://reviews.llvm.org/D129149
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
