[PATCH] D114379: [OMPIRBuilder] Add support for simd (loop) directive.

[Michael Kruse via Phabricator via llvm-commits]

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