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

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur requested changes to this revision.
Meinersbur added inline comments.
This revision now requires changes to proceed.


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2588-2594
+    if (isa<OMPIfClause>(C) || isa<OMPSafelenClause>(C) ||
+        isa<OMPSimdlenClause>(C) || isa<OMPLinearClause>(C) ||
+        isa<OMPAlignedClause>(C) || isa<OMPNontemporalClause>(C) ||
+        isa<OMPPrivateClause>(C) || isa<OMPLastprivateClause>(C) ||
+        isa<OMPReductionClause>(C) || isa<OMPCollapseClause>(C) ||
+        isa<OMPOrderClause>(C))
+      return false;
----------------
I think it would be better to list the clauses that are supported. In addition to being shorter, it would also reject any clause we forgot about and new ones.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2123
+                            ArrayRef<Metadata *> Properties) {
+  for (auto &I : *Block) {
+    if (I.mayReadFromMemory() || I.mayWriteToMemory()) {
----------------
[style] According to the LLVM coding standard, auto is only used in specific cases.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2125
+    if (I.mayReadFromMemory() || I.mayWriteToMemory()) {
+      Instruction *instr = dyn_cast<Instruction>(&I);
+      LLVMContext &C = instr->getContext();
----------------
[style] Please use the current LLVM coding style.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2127
+      LLVMContext &C = instr->getContext();
+      MDNode *LoopID = MDNode::get(C, Properties);
+      instr->setMetadata("llvm.access.group", LoopID);
----------------
[serious] This does not create a LoopID. A LoopID is a bag of properties where the first element points to itself (which automatically makes then `distinct`). E.g.
```
!0 = distinct !{!0, !{!"llvm.loop.parallel_accesses", !1}}
```
where `!1` is an access group (eg `!1 = distinct !{}`).

[serious] The instruction's metadata should not be a LoopID, but a access group (eg `!1` for the above example). I think you should not pass an array of properties, but just the access group MDNode.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2152
+      Loop,
+      {MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.parallel_accesses")),
+       MDNode::get(Ctx, MDString::get(Ctx, "llvm.loop.vectorize.enable"))});
----------------
[serious] The `llvm.loop.parallel_accesses` property takes an argument: the access group.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2157-2158
+  // exit block.  May have to enhance this collection for nested loops.
+  BasicBlock *body = Loop->getBody();
+  BasicBlock *exit = Loop->getExit();
+
----------------
[Style] Please use the current LLVM coding standard.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2182
+        worklist.push_back(succ);
+        if (!DT.dominates(exit, succ) && skipBBs.count(succ) == 0)
+          reachable.insert(succ);
----------------
[serious] Using DominatorTree to check whether we passes Exit is redundant: For any block that is dominated by Exit (by definition of dominator) we will have to visited Exit first anyway. Therefore not adding Exit itself would be sufficient.

You actually don't want paths that go to exit, but the basic blocks that come from Body (the successor of Cond) to the Continue/Latch block. By the definition of CanonicalLoopInfo, any block reachable from Body can only go to Continue[1], so it would be sufficient to iterate everything reachable from Body, including Body itself, until we hit the Latch. Header and Cond themselves are not allowed to contain any memory accesses.

The safest way to determine the set of blocks inside the loop would be using LoopInfo, which internally uses DominatorTree, but for checking dominance with the header, not the exit.

[1] There are exceptions: paths can end in an `unreachable` instruction and I am currently working on defining what happens at cancellation[2]. There will also be a BasicBlock like Latch/Continue that signals the end of the loop. In both cases these still contain blocks that are not inside the loop in LoopInfo's sense. However, marking instructions in them with access groups should is not a problem since the access groups marker only has an effect if inside a loop that has the `llvm.loop.parallel_accesses` property set to that access group.

[2] I found interesting cases in which Clang re-uses the same basic blocks (cleanup block for destructors and exception handling) for multiple paths by using a PHINode and a switch to branch to the scope it came from. That is, if part of a loop it would always branch back to another block in the loop, but statically any other switch target would be reachable as well. Not sure yet whether this will be a problem that requires us to use LoopInfo.


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

https://reviews.llvm.org/D114379