Implementation of the unroll directive introduced in OpenMP 5.1. Follows the approach from D76342 for the tile directive (i.e. AST-based, not using the OpenMPIRBuilder). Tries to use llvm.loop.unroll.* metadata where possible, but has to fall back to an AST representation of the outer loop if the partially unrolled generated loop is associated with another directive (because it needs to compute the number of iterations).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/include/clang/AST/OpenMPClause.h | ||
---|---|---|
899 | sizes->full | |
902 | Same | |
903 | No param | |
904 | Wrong description | |
905 | No param | |
912 | No param | |
946 | sizes->partial | |
948–952 | Fix params descriptions | |
957–960 | Description | |
971 | Make it private, if possible | |
clang/include/clang/AST/StmtOpenMP.h | ||
5070 | Description | |
5094–5105 | Fix description | |
5111–5115 | Description | |
clang/lib/AST/StmtProfile.cpp | ||
466 | Unelated change? | |
474 | const Expr * | |
clang/lib/CodeGen/CGStmtOpenMP.cpp | ||
2591 | getSingleClause or hasClausesOfKind | |
2598 | getSingleClause | |
2607 | const Expr * | |
2608–2611 | I suppose it is compiled-time expression, right? If so, use FactorExpr->EvaluateKnownConstInt() or something similar. | |
clang/lib/Parse/ParseOpenMP.cpp | ||
2985–2986 | Better to remove the default: case completely | |
clang/lib/Sema/SemaOpenMP.cpp | ||
12560 | I think this function can be made static local as it is used only SemaOpenMP.cpp | |
12825 | getSingleClause | |
12828 | Add a text message to the assert. | |
12833 | getSingleClause | |
12851 | Need to remove | |
12858–12860 | I think this should be checked before the first OMPUnrollDirective::Create call | |
12934 | unroll? | |
14965 | Need to pass factor expr and real source loc | |
clang/lib/Serialization/ASTReaderStmt.cpp | ||
3196 | Add a message for the assert |
Some previous comments were not addressed yet
clang/include/clang/AST/OpenMPClause.h | ||
---|---|---|
972–974 | Make it private | |
clang/lib/AST/StmtOpenMP.cpp | ||
141 | Enclose in braces | |
clang/lib/CodeGen/CGStmtOpenMP.cpp | ||
2602–2606 | I suppose it is compiled-time expression, right? If so, use FactorExpr->EvaluateKnownConstInt() or something similar. |
clang/include/clang/AST/OpenMPClause.h | ||
---|---|---|
907 | Fix params descriptions | |
clang/include/clang/AST/StmtOpenMP.h | ||
463–478 | Better to commit it it separately as NFC. | |
clang/lib/AST/StmtOpenMP.cpp | ||
382 | auto * | |
clang/lib/CodeGen/CGStmtOpenMP.cpp | ||
2594–2598 | Can we have full and partial at the same time? If no, emit partial in else substatement for full clause. | |
clang/lib/Sema/SemaOpenMP.cpp | ||
8984–8985 | Enclose in braces | |
10139–10142 | Would be good if this change is committed separately as NFC. | |
12604 | assert message? | |
12821 | Can you use VerifyPositiveIntegerConstantInClause function instead? | |
12955–12956 | Better to use PartialClause->getFactor()->isIntegerConstantExpr(Ctx) or something similar. | |
12964 | Why do you need it? Just use original PartialClause->getFactor() | |
12985 | MakeOuterRef | |
12993 | MakeInnerRef | |
13000 | MakeNumIterations |
clang/include/clang/AST/StmtOpenMP.h | ||
---|---|---|
463–478 | ||
clang/lib/Sema/SemaOpenMP.cpp | ||
10139–10142 | ||
12821 | The reason for this new function is that VerifyPositiveIntegerConstantInClause also emits a note on why it is not a constant expression: note: read of non-const variable '.capture_expr.' is not allowed in a constant expression Printing internal variable names is more confusing than helpful to the user. | |
12964 | The AST invariant that no ASTNode can be used multiple times (within the same DeclContext) must be preserved. This is a utility lambda (like the other MakeXyz) that create a new IntegerLiteral node for ever use. See discussion here: https://reviews.llvm.org/D94973?id=322600#inline-905341 |
clang/lib/Sema/SemaOpenMP.cpp | ||
---|---|---|
12821 | Maybe expand VerifyPositiveIntegerConstantInClause to exclude this rather than add a new function? |
clang/lib/CodeGen/CGStmtOpenMP.cpp | ||
---|---|---|
2602–2606 | Not done. |
clang/lib/CodeGen/CGStmtOpenMP.cpp | ||
---|---|---|
2602–2606 | It was done in another branch that I forgot to merge. Sorry. |
sizes->full