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
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
2589

getSingleClause or hasClausesOfKind

2596

getSingleClause

2605

const Expr *

2606–2609

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

clang/lib/Parse/ParseOpenMP.cpp
2973–2974

Better to remove the default: case completely

clang/lib/Sema/SemaOpenMP.cpp
12558

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

12822

getSingleClause

12825

Add a text message to the assert.

12830

getSingleClause

12848

Need to remove

12855–12857

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

12931

unroll?

14963

Need to pass factor expr and real source loc

clang/lib/Serialization/ASTReaderStmt.cpp
3194

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
141

Enclose in braces

clang/lib/CodeGen/CGStmtOpenMP.cpp
2600–2604

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
463–479

Better to commit it it separately as NFC.

clang/lib/AST/StmtOpenMP.cpp
381

auto *

clang/lib/CodeGen/CGStmtOpenMP.cpp
2592–2596

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
8982–8983

Enclose in braces

10137–10141

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

12602

assert message?

12818

Can you use VerifyPositiveIntegerConstantInClause function instead?

12952–12953

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

12961

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

12982

MakeOuterRef

12990

MakeInnerRef

12997

MakeNumIterations

Meinersbur marked 8 inline comments as done.Jun 3 2021, 9:35 PM
Meinersbur added inline comments.
clang/include/clang/AST/StmtOpenMP.h
463–479
clang/lib/Sema/SemaOpenMP.cpp
10137–10141
12818

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.

12961

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
12818

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
2600–2604

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
2600–2604

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.