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

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

[testing] Could you add a test for `applySimd` into `OpenMPIRBuilderTests.cpp`, so we have a test that is independent of Clang?



================
Comment at: clang/test/OpenMP/irbuilder_simd.cpp:3
+// expected-no-diagnostics 
+// RUN: %clang_cc1 -fopenmp-enable-irbuilder -verify -fopenmp -fopenmp-version=45 -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s -check-prefix=CHECKTWOLOOPS 
+// expected-no-diagnostics
----------------
The invocation is identical to the RUN on line 1. `CHECKTWOLOOPS` is not needed and can also be made as `CHECK`.


================
Comment at: clang/test/OpenMP/irbuilder_simd.cpp:4
+// RUN: %clang_cc1 -fopenmp-enable-irbuilder -verify -fopenmp -fopenmp-version=45 -x c++ -triple x86_64-unknown-unknown -emit-llvm %s -o - | FileCheck %s -check-prefix=CHECKTWOLOOPS 
+// expected-no-diagnostics
+
----------------
A single `expected-no-diagnostics` is sufficient


================
Comment at: clang/test/OpenMP/irbuilder_simd.cpp:67-69
+// CHECK: ![[META0:[0-9]+]] = !{i32 1, !"wchar_size", i32 4}
+// CHECK-NEXT: ![[META1:[0-9]+]]  = !{i32 7, !"openmp", i32 45}
+// CHECK-NEXT: ![[META2:[0-9]+]]  =
----------------
If you added these CHECK lines by hand, remove those that are not relevant


================
Comment at: clang/test/OpenMP/irbuilder_simd.cpp:71
+// CHECK-NEXT: ![[META3:[0-9]+]] = distinct !{}
+// CHECK-NEXT: ![[META4:[0-9]+]]  = distinct !{![[META4:[0-9]+]], ![[META5:[0-9]+]], ![[META6:[0-9]+]]}
+// CHECK-NEXT: ![[META5:[0-9]+]]  = !{!"llvm.loop.parallel_accesses", ![[META3:[0-9]+]]}
----------------
Do not add a regex specification for any but the first occurance. A regex specification will "overwrite" the previous match, i.e. the different occurrences of `META4` would not need to be the same.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2120
 
+/// Attach metadata llvm.access.group to the memref instructions of \p block
+static void addSimdMetadata(BasicBlock *Block,
----------------



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2124
+  for (Instruction &I : *Block) {
+    if (I.mayReadFromMemory() || I.mayWriteToMemory()) {
+      I.setMetadata(LLVMContext::MD_access_group, AccessGroup);
----------------



================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2125
+    if (I.mayReadFromMemory() || I.mayWriteToMemory()) {
+      I.setMetadata(LLVMContext::MD_access_group, AccessGroup);
+    }
----------------
The instruction may already have an access group assigned e.g. from a nested `#pragma clang loop vectorize(assume_safety)`. This would overwrite the access group. See `LoopInfoStack::InsertHelper` (`CGLoopInfo.cpp`) on how to assign multiple access groups, or add a TODO.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2156
+  DominatorTreeAnalysis DTA;
+  DominatorTree &&DT = DTA.run(*F, FAM);
+  LoopAnalysis LIA;
----------------
Compiler warning: `warning C4189: 'DT': local variable is initialized but not referenced`


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2162
+
+  llvm::SmallSet<BasicBlock *, 8> Reachable; 
+
----------------
OpenMPIRBuilder is part of the `llvm` namespace, `llvm::` is not necessary.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2166
+  // can be found.
+  for (BasicBlock *Block:L->getBlocks()) {
+    if (Block == CanonicalLoop->getCond() || Block == CanonicalLoop->getHeader()) continue;
----------------
Could you add a todo note such as: 
```
// TODO: Generalize getting all blocks inside a CanonicalizeLoopInfo, preferably without running any passes.
```


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2173-2175
+  for (BasicBlock *BB : Reachable) {
+    addSimdMetadata(BB, AccessGroup);
+  }
----------------
[style]


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2181
+          ConstantInt::getTrue(Type::getInt1Ty(Ctx)));
+  addLoopMetadata(
+      CanonicalLoop,
----------------
The loop might already have a `llvm.loop.parallel_accesses`, in which case those two lists could be combined.

I don't think its very relevant, but maybe add a TODO?


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

https://reviews.llvm.org/D114379