[PATCH] D129149: [OMPIRBuilder] Add support for simdlen clause

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Generally, this is fine. Some nits. The only reason not to accept this right now is the test. Why manual check lines?
Wrt. the function signature. As soon as we have more then SIMD directives to check we refactor things. Keep it simple
until you need the functionality.



================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2598
+    // Currently only simdlen clause is supported
+    if (!dyn_cast<OMPSimdlenClause>(C))
+      return false;
----------------



================
Comment at: clang/test/OpenMP/irbuilder_simdlen.cpp:1
+// RUN: %clang_cc1 -no-opaque-pointers -fopenmp-enable-irbuilder -verify -fopenmp -fopenmp-version=45 -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s
+// expected-no-diagnostics
----------------
The check lines look auto-generated and then modified by hand. Why is that?


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:608
+  ///
+  /// \param DL      Debug location for instructions added by unrolling.
+  /// \param Loop    The simd loop.
----------------
No debug location needed. You also copied the comment that makes little sene.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2885
+void OpenMPIRBuilder::applySimdlen(DebugLoc, CanonicalLoopInfo *CanonicalLoop,
+                                   llvm::ConstantInt *Simdlen) {
+  LLVMContext &Ctx = Builder.getContext();
----------------
Also above, no llvm::


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

https://reviews.llvm.org/D129149