This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Implement '#pragma omp unroll'.
ClosedPublic

Authored by Meinersbur on Mar 27 2021, 4:33 PM.

Details

Summary

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).

Diff Detail

Event Timeline

Meinersbur created this revision.Mar 27 2021, 4:33 PM
Meinersbur requested review of this revision.Mar 27 2021, 4:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2021, 4:33 PM
ABataev added inline comments.
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
5037

Description

5061–5072

Fix description

5078–5082

Description

clang/lib/AST/StmtProfile.cpp
466

Unelated change?

474

const Expr *

clang/lib/CodeGen/CGStmtOpenMP.cpp
2573

getSingleClause or hasClausesOfKind

2580

getSingleClause

2589

const Expr *

2590–2593

I suppose it is compiled-time expression, right? If so, use FactorExpr->EvaluateKnownConstInt() or something similar.

clang/lib/Parse/ParseOpenMP.cpp
2948–2949

Better to remove the default: case completely

clang/lib/Sema/SemaOpenMP.cpp
12451

I think this function can be made static local as it is used only SemaOpenMP.cpp

12691

getSingleClause

12694

Add a text message to the assert.

12699

getSingleClause

12717

Need to remove

12724–12726

I think this should be checked before the first OMPUnrollDirective::Create call

12800

unroll?

14728

Need to pass factor expr and real source loc

clang/lib/Serialization/ASTReaderStmt.cpp
3179

Add a message for the assert

  • Ready version
  • Undo spurious change
Meinersbur retitled this revision from [OpenMP] Implement '#pragma omp unroll'. WIP. to [OpenMP] Implement '#pragma omp unroll'..May 12 2021, 9:09 PM
Meinersbur edited the summary of this revision. (Show Details)

Removed [WIP] flag; ready to review.

Meinersbur marked 29 inline comments as done.May 12 2021, 9:11 PM

Some previous comments were not addressed yet

clang/include/clang/AST/OpenMPClause.h
972–974

Make it private

clang/lib/AST/StmtOpenMP.cpp
138

Enclose in braces

clang/lib/CodeGen/CGStmtOpenMP.cpp
2584–2588

I suppose it is compiled-time expression, right? If so, use FactorExpr->EvaluateKnownConstInt() or something similar.

Meinersbur marked 3 inline comments as done.
  • Address review
ABataev added inline comments.Jun 2 2021, 7:24 AM
clang/include/clang/AST/OpenMPClause.h
907

Fix params descriptions

clang/include/clang/AST/StmtOpenMP.h
464–474

Better to commit it it separately as NFC.

clang/lib/AST/StmtOpenMP.cpp
369

auto *

clang/lib/CodeGen/CGStmtOpenMP.cpp
2576–2580

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
8944–8945

Enclose in braces

10017–10018

Would be good if this change is committed separately as NFC.

12495

assert message?

12687

Can you use VerifyPositiveIntegerConstantInClause function instead?

12821–12822

Better to use PartialClause->getFactor()->isIntegerConstantExpr(Ctx) or something similar.

12830

Why do you need it? Just use original PartialClause->getFactor()

12851

MakeOuterRef

12859

MakeInnerRef

12866

MakeNumIterations

Meinersbur marked 8 inline comments as done.Jun 3 2021, 9:35 PM
Meinersbur added inline comments.
clang/include/clang/AST/StmtOpenMP.h
464–474
clang/lib/Sema/SemaOpenMP.cpp
10017–10018
12687

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.

12830

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

Meinersbur updated this revision to Diff 349765.Jun 3 2021, 9:36 PM
  • Address review
ABataev added inline comments.Jun 4 2021, 8:36 AM
clang/lib/Sema/SemaOpenMP.cpp
12687

Maybe expand VerifyPositiveIntegerConstantInClause to exclude this rather than add a new function?

Meinersbur updated this revision to Diff 350748.Jun 8 2021, 5:25 PM
  • Integreate isConstantExpression into VerifyPositiveIntegerConstantInClause
cchen added a subscriber: cchen.Jun 9 2021, 9:22 AM
ABataev added inline comments.Jun 10 2021, 5:35 AM
clang/lib/CodeGen/CGStmtOpenMP.cpp
2584–2588

Not done.

  • Merge branch 'arcpatch-D99459' into pragma-omp-unroll
Meinersbur marked an inline comment as done.Jun 10 2021, 11:43 AM
Meinersbur added inline comments.
clang/lib/CodeGen/CGStmtOpenMP.cpp
2584–2588

It was done in another branch that I forgot to merge. Sorry.

This revision is now accepted and ready to land.Jun 10 2021, 11:51 AM
This revision was landed with ongoing or failed builds.Jun 10 2021, 12:34 PM
This revision was automatically updated to reflect the committed changes.
Meinersbur marked an inline comment as done.