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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
