Meinersbur added inline comments.
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2586-2587
void CodeGenFunction::EmitOMPSimdDirective(const OMPSimdDirective &S) {
+
+ bool UseOMPIRBuilder = CGM.getLangOpts().OpenMPIRBuilder;
+ if (UseOMPIRBuilder) {
----------------
arnamoy10 wrote:
> Meinersbur wrote:
> > [nit] Unnecessary whitespace
> I took a look at `isSupportedByOpenMPIRBuilder()` and seems like it is more
> suitable for a check against work-sharing loop construct, not for `simd`.
>
> And for `simd`, I am not sure what features I should be checking for. One
> check I can think of is checking whether any of the not-yet-supported clauses
> is present in the simd directive or not. Am I on the right track?
Indeed, `isSupportedByOpenMPIRBuilder` is for the workshoring-loop, you could
introduce another one for simd. Or modify it to accept `OMPExecutableDirective`
as an argument. This might be useful for combined constructs such as `for simd`.
Best approach IMHO is to whitelist clauses. Reject any clause you did not
test/add support yet. This way we can fallback to the non-OpenMPIRBuilder
codegen and not break things that worked without `-fopenmp-enable-irbuilder` (a
lot of code already does not work `-fopenmp-enable-irbuilder`, so don't put too
much effort into getting the interaction between OpenMPIRBuilder and current
codegen to work; but we need an overview what is already supported and which
clauses are still require some work)
================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2594
+ // Emit the associated statement and get its loop representation.
+ auto DL = SourceLocToDebugLoc(S.getBeginLoc());
+ const Stmt *Inner = S.getRawStmt();
----------------
arnamoy10 wrote:
> Meinersbur wrote:
> > [style] According to the [[
> > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> > | LLVM coding standard ]], `auto` is only used in specific cases.
> I am inspired by [[
> https://github.com/llvm/llvm-project/blob/003c9c7457d08888be5deeca7eee84ab5f110bf6/clang/lib/CodeGen/CGStmtOpenMP.cpp#L2611
> | existing code ]] though
There already is a lot of code violating the coding standard, either because
the coding standard changed or it slipped through a review. A prominent example
is that the methods of the IRBuilder are still PascalCase, although the current
standard says they should use camelCase. Same applies to `SourceLocToDebugLoc`
itself (which every time I see it makes me think `SourceLocToDebugLoc` is a
type)
Not a reason to not use the newest coding standard for new code.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D114379/new/
https://reviews.llvm.org/D114379
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
