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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur requested changes to this revision.
Meinersbur added a comment.
This revision now requires changes to proceed.

Thanks for helping to complete the OpenMPIRBuilder implementation!



================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2586-2587
 void CodeGenFunction::EmitOMPSimdDirective(const OMPSimdDirective &S) {
+
+  bool UseOMPIRBuilder = CGM.getLangOpts().OpenMPIRBuilder;
+  if (UseOMPIRBuilder) {
----------------
[nit] Unnecessary whitespace


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2587
+
+  bool UseOMPIRBuilder = CGM.getLangOpts().OpenMPIRBuilder;
+  if (UseOMPIRBuilder) {
----------------
Did you consider adding a check whether OpenMPIRBuilder actually implements all features requested before using it? See `isSupportedByOpenMPIRBuilder`.


================
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();
----------------
[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.


================
Comment at: clang/test/OpenMP/simd_codegen_irbuilder.cpp:16
+    // CHECK-NEXT: call void @__captured_stmt.1(i32* %i, i32 %omp_loop.iv, %struct.anon.0* %agg.captured1)
+    // CHECK-NEXT: %3 = load float*, float** %b.addr, align 8, !llvm.access.group !3
+    // CHECK-NEXT: %4 = load i32, i32* %i, align 4, !llvm.access.group !3
----------------
Could you use regexes to match the virtual register names? The script `update_cc_checks.py` may help with this, see the other tests.

The naming scheme for files testing the OpenMPIRBiulder is usually `irbuilder_<constructs>`


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:524
+  /// \param Loop The loop to simd-ise.
+  void createSIMDLoop(DebugLoc DL, CanonicalLoopInfo *Loop);
+
----------------
Capitalizing SIMD leads to long runs of capital letters. While not an LLVM coding style rule, it is still common to title-case acronyms in identifiers if the acronym is ~4 letters or more, such as `OMPSimdDirective` instead of `OMPSIMDDirective`.

For methods transformation `CanonicalLoopInfo`, we don't use `createXYZ` naming scheme which is reserved for inserting something new. Suggestions: `applySimd`, `vectorizeLoop`, or `simdizeLoop`.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2120
+/// Attach metadata access.group to the load and store instructions of \p block
+static void addSIMDMetadata(BasicBlock *block,
+                            ArrayRef<Metadata *> Properties) {
----------------
The LLVM coding style still capitalizes parameters and variables.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2123
+  for (auto &I : *block) {
+    if (isa<LoadInst>(&I) || isa<StoreInst>(&I)) {
+      Instruction *instr = dyn_cast<Instruction>(&I);
----------------
The property also applies to any memory access, e.g. `memset`. You could use `mayWriteToMemory()` and `mayReadFromMemory()`.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2126
+      LLVMContext &C = instr->getContext();
+      MDNode *N = MDNode::get(C, MDString::get(C, ""));
+      instr->setMetadata("llvm.access.group", N);
----------------
[serious] The access group must be unique only to the loop being accessed.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2157-2168
+  addSIMDMetadata(header,
+                  {
+                      MDNode::get(Ctx, MDString::get(Ctx, "llvm.access.group")),
+                  });
+  addSIMDMetadata(cond,
+                  {
+                      MDNode::get(Ctx, MDString::get(Ctx, "llvm.access.group")),
----------------
[serious] header and cond don't contain any memory accesses. `body` may consist of multiple basic blocks, of which this only applies to the first one. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114379