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

  • 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
2602–2606

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

Meinersbur marked 8 inline comments as done.Jun 3 2021, 9:35 PM
Meinersbur added inline comments.
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

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
12821

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
2602–2606

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
2602–2606

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.