[PATCH] D129149: [OMPIRBuilder] Add support for simdlen clause
Johannes Doerfert via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 6 08:43:36 PDT 2022
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
More information about the llvm-commits
mailing list