Meinersbur added a comment.
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.
================
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:
> > > 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.
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
