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