[Clang][OpenMP] Support for dispatch construct (Sema & Codegen) support #131838
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4529,6 +4529,191 @@ void CodeGenFunction::EmitOMPMasterDirective(const OMPMasterDirective &S) { | |
| emitMaster(*this, S); | ||
| } | ||
|
|
||
| static Expr *replaceWithNewTraitsOrDirectCall(CapturedDecl *CDecl, | ||
| Expr *NewExpr) { | ||
| Expr *CurrentCallExpr = nullptr; | ||
| Stmt *CallExprStmt = CDecl->getBody(); | ||
|
|
||
| if (BinaryOperator *BinaryCopyOpr = dyn_cast<BinaryOperator>(CallExprStmt)) { | ||
| CurrentCallExpr = BinaryCopyOpr->getRHS(); | ||
| BinaryCopyOpr->setRHS(NewExpr); | ||
| } else { | ||
| CurrentCallExpr = dyn_cast<Expr>(CallExprStmt); | ||
| CDecl->setBody(NewExpr); | ||
| } | ||
|
|
||
| return CurrentCallExpr; | ||
| } | ||
|
|
||
| static Expr *transformCallInStmt(Stmt *StmtP, bool NoContext = false) { | ||
| Expr *CurrentExpr = nullptr; | ||
| if (auto *CptStmt = dyn_cast<CapturedStmt>(StmtP)) { | ||
| CapturedDecl *CDecl = CptStmt->getCapturedDecl(); | ||
|
|
||
| CallExpr *NewCallExpr = nullptr; | ||
| for (const auto *attr : CDecl->attrs()) { | ||
| if (NoContext) { | ||
| if (const auto *annotateAttr = | ||
| llvm::dyn_cast<clang::AnnotateAttr>(attr); | ||
| annotateAttr && annotateAttr->getAnnotation() == "NoContextAttr") { | ||
| NewCallExpr = llvm::dyn_cast<CallExpr>(*annotateAttr->args_begin()); | ||
| } | ||
| } else { | ||
| if (const auto *annotateAttr = | ||
| llvm::dyn_cast<clang::AnnotateAttr>(attr); | ||
| annotateAttr && annotateAttr->getAnnotation() == "NoVariantsAttr") { | ||
| NewCallExpr = llvm::dyn_cast<CallExpr>(*annotateAttr->args_begin()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| CurrentExpr = replaceWithNewTraitsOrDirectCall(CDecl, NewCallExpr); | ||
| } | ||
| return CurrentExpr; | ||
| } | ||
|
|
||
| // emitIfElse is used for the following conditions: | ||
| // | ||
| // NoVariants = 0 && NoContext = 1 | ||
| // if (Condition_NoContext) { | ||
| // foo_variant2(); // Present in AnnotationAttr | ||
| // } else { | ||
| // foo_variant(); | ||
| // } | ||
| // | ||
| // NoVariants = 1 && NoContext = 0 | ||
| // if (Condition_NoVariants) { | ||
| // foo(); | ||
| // } else { | ||
| // foo_variant(); | ||
| // } | ||
| // | ||
| // NoVariants = 1 && NoContext = 1 | ||
| // if (Condition_NoVariants) { // ==> label if.then.NoVariants | ||
| // foo(); | ||
| // } else { // ==> label else.NoVariants | ||
| // if (Condition_NoContext) { // ==> label if.then.NoContext | ||
| // foo_variant2(); // Present in AnnotationAttr | ||
| // } else { // ==> label else | ||
| // foo_variant(); | ||
| // } | ||
| // } | ||
| // | ||
| static void emitIfElse(CodeGenFunction *CGF, Stmt *AssociatedStmt, | ||
|
Member
There was a problem hiding this comment. I rather doubt it will correctly with constructors/destructors
Contributor
Author
There was a problem hiding this comment. Do you have an example of constructors/destructors where it will not work? The codegen for the
Member
There was a problem hiding this comment. Try to add tests with classes, which evaluate to boolean, and create immediate class instance and see, that the destructors are called correctly
Contributor
There was a problem hiding this comment. @alexey-bataev Both @SunilKuravinakop and I are unclear what test you're asking for here. When you say "test with classes", do you mean that the called function (and its variant) should have a local declaration of a variable of class type? Such as: void f_v()
{
MyType2 v; // MyType2 constructor called on v
...
// MyType2 destructor called on v
}
#pragma omp declare variant(f_v) match(construct={dispatch})
void f()
{
MyType v; // MyType constructor called on v
...
// MyType destructor called on v
}
int main()
{
#pragma omp dispatch
f();
}But then, what do you mean by "which evaluate to boolean"? Can you provide a quick sketch of what this test should be doing?
Member
There was a problem hiding this comment. No, I thought about the condition, which involves instantiation of the class, to check that the codegen handles correctly constructors/destructors calls (that's always a problem in C++). Is it possible to create a condition, that includes class, something like:
Contributor
There was a problem hiding this comment. Thanks for the clarification. So something like this? #include <iostream>
int main()
{
class IsPositive {
public:
IsPositive(int x) : _x(x) {
std::cout << "constructing IsPositive(" << x << ") object\n";
}
~IsPositive() {
std::cout << "destructing IsPositive(" << _x << ") object\n";
}
operator bool() const { return _x > 0; }
private:
int _x;
};
// case 1
#pragma omp parallel if(IsPositive(10))
{
std::cout << "[1] in parallel\n";
}
// case 2
auto cond1 = IsPositive(5);
#pragma omp parallel if(cond1)
{
std::cout << "[2] in parallel\n";
}
// case 3
#pragma omp parallel if(IsPositive(-10))
{
std::cout << "[3] in parallel\n";
}
// case 4
auto cond2 = IsPositive(-5);
#pragma omp parallel if(cond2)
{
std::cout << "[4] in parallel\n";
}
The output for the above with So, the destructor for the instantiated class object used in the Does this match the expected behavior?
Member
There was a problem hiding this comment. Yes, something like this but for dispatch. We need to handle classes construction/deconstruction correctly, I just want to be sure that we don't miss anything here and avoid bugs, that will require significant efforts to fix/rework later. |
||
| Expr *Condition_NoVariants, Expr *Condition_NoContext) { | ||
| llvm::BasicBlock *ThenBlock = CGF->createBasicBlock("if.then"); | ||
| llvm::BasicBlock *ElseBlock = CGF->createBasicBlock("if.else"); | ||
| llvm::BasicBlock *MergeBlock = CGF->createBasicBlock("if.end"); | ||
| llvm::BasicBlock *ThenNoVariantsBlock = nullptr; | ||
| llvm::BasicBlock *ElseNoVariantsBlock = nullptr; | ||
| llvm::BasicBlock *ThenNoContextBlock = nullptr; | ||
| Expr *ElseCall = nullptr; | ||
|
|
||
| if (Condition_NoVariants && Condition_NoContext) { | ||
| ThenNoVariantsBlock = CGF->createBasicBlock("if.then.NoVariants"); | ||
| ElseNoVariantsBlock = CGF->createBasicBlock("else.NoVariants"); | ||
| ThenNoContextBlock = CGF->createBasicBlock("if.then.NoContext"); | ||
|
|
||
| CGF->EmitBranchOnBoolExpr(Condition_NoVariants, ThenNoVariantsBlock, | ||
| ElseNoVariantsBlock, 0); | ||
|
|
||
| } else if (Condition_NoVariants) | ||
| CGF->EmitBranchOnBoolExpr(Condition_NoVariants, ThenBlock, ElseBlock, 0); | ||
| else | ||
| CGF->EmitBranchOnBoolExpr(Condition_NoContext, ThenBlock, ElseBlock, 0); | ||
SunilKuravinakop marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| if (Condition_NoVariants && Condition_NoContext) { | ||
| // Emit the NoVariants (if then, for the NoVariants) block. | ||
| CGF->EmitBlock(ThenNoVariantsBlock); | ||
| Stmt *ThenStmt = AssociatedStmt; | ||
| ElseCall = transformCallInStmt(ThenStmt, false); | ||
| CGF->EmitStmt(ThenStmt); | ||
| CGF->Builder.CreateBr(MergeBlock); | ||
|
|
||
| CGF->EmitBlock(ElseNoVariantsBlock); | ||
| CGF->EmitBranchOnBoolExpr(Condition_NoVariants, ThenNoContextBlock, | ||
| ElseBlock, 0); | ||
| // Emit the NoContext (else if, for the NoContext) block. | ||
| CGF->EmitBlock(ThenNoContextBlock); | ||
| Stmt *ThenNoContextStmt = AssociatedStmt; | ||
| transformCallInStmt(ThenNoContextStmt, true); | ||
| CGF->EmitStmt(ThenNoContextStmt); | ||
| CGF->Builder.CreateBr(MergeBlock); | ||
|
|
||
| } else if (Condition_NoVariants) { | ||
| // Emit the NoVariants (then) block. | ||
| CGF->EmitBlock(ThenBlock); | ||
| Stmt *ThenStmt = AssociatedStmt; | ||
| ElseCall = transformCallInStmt(ThenStmt, false); | ||
| CGF->EmitStmt(ThenStmt); | ||
| CGF->Builder.CreateBr(MergeBlock); | ||
|
|
||
| } else if (Condition_NoContext) { | ||
| // Emit the NoContext (then) block. | ||
| CGF->EmitBlock(ThenBlock); | ||
| Stmt *ThenStmt = AssociatedStmt; | ||
| ElseCall = transformCallInStmt(ThenStmt, true); | ||
| CGF->EmitStmt(ThenStmt); | ||
| CGF->Builder.CreateBr(MergeBlock); | ||
| } | ||
SunilKuravinakop marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| // Emit the else block. | ||
| CGF->EmitBlock(ElseBlock); | ||
| Stmt *ElseStmt = AssociatedStmt; | ||
| if (auto *CaptStmt = dyn_cast<CapturedStmt>(ElseStmt)) { | ||
| CapturedDecl *CDecl = CaptStmt->getCapturedDecl(); | ||
| replaceWithNewTraitsOrDirectCall(CDecl, ElseCall); | ||
| } | ||
| CGF->EmitStmt(ElseStmt); | ||
| CGF->Builder.CreateBr(MergeBlock); | ||
|
|
||
| CGF->EmitBlock(MergeBlock); | ||
| } | ||
|
|
||
| void CodeGenFunction::EmitOMPDispatchDirective(const OMPDispatchDirective &S) { | ||
| ArrayRef<OMPClause *> Clauses = S.clauses(); | ||
|
|
||
| Stmt *AssociatedStmt = const_cast<Stmt *>(S.getAssociatedStmt()); | ||
| if (auto *AssocStmt = dyn_cast<CapturedStmt>(AssociatedStmt)) | ||
| if (auto *InnerCapturedStmt = | ||
| dyn_cast<CapturedStmt>(AssocStmt->getCapturedStmt())) { | ||
| AssociatedStmt = InnerCapturedStmt; | ||
| } | ||
SunilKuravinakop marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| CodeGenFunction::CGCapturedStmtInfo CapStmtInfo; | ||
| if (!CapturedStmtInfo) | ||
| CapturedStmtInfo = &CapStmtInfo; | ||
|
|
||
| Expr *NoVariantsCondition = nullptr; | ||
| Expr *NoContextCondition = nullptr; | ||
| if (!Clauses.empty()) { | ||
| if (S.hasClausesOfKind<OMPDependClause>()) | ||
| EmitOMPDispatchToTaskwaitDirective(S); | ||
SunilKuravinakop marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| if (S.hasClausesOfKind<OMPNovariantsClause>() || | ||
| S.hasClausesOfKind<OMPNocontextClause>()) { | ||
| if (const OMPNovariantsClause *NoVariantsC = | ||
| OMPExecutableDirective::getSingleClause<OMPNovariantsClause>( | ||
| Clauses)) { | ||
| NoVariantsCondition = NoVariantsC->getCondition(); | ||
| if (const OMPNocontextClause *NoContextC = | ||
| OMPExecutableDirective::getSingleClause<OMPNocontextClause>( | ||
| Clauses)) { | ||
| NoContextCondition = NoContextC->getCondition(); | ||
| } | ||
| } else { | ||
| const OMPNocontextClause *NoContextC = | ||
| OMPExecutableDirective::getSingleClause<OMPNocontextClause>( | ||
| Clauses); | ||
| NoContextCondition = NoContextC->getCondition(); | ||
| } | ||
|
|
||
| OMPLexicalScope Scope(*this, S, OMPD_dispatch); | ||
| emitIfElse(this, AssociatedStmt, NoVariantsCondition, NoContextCondition); | ||
| } | ||
| } else | ||
| EmitStmt(AssociatedStmt); | ||
SunilKuravinakop marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| static void emitMasked(CodeGenFunction &CGF, const OMPExecutableDirective &S) { | ||
| auto &&CodeGen = [&S](CodeGenFunction &CGF, PrePostActionTy &Action) { | ||
| Action.Enter(CGF); | ||
|
|
@@ -5549,6 +5734,15 @@ void CodeGenFunction::EmitOMPBarrierDirective(const OMPBarrierDirective &S) { | |
| CGM.getOpenMPRuntime().emitBarrierCall(*this, S.getBeginLoc(), OMPD_barrier); | ||
| } | ||
|
|
||
| void CodeGenFunction::EmitOMPDispatchToTaskwaitDirective( | ||
| const OMPDispatchDirective &S) { | ||
| OMPTaskDataTy Data; | ||
| // Build list of dependences | ||
| buildDependences(S, Data); | ||
| Data.HasNowaitClause = S.hasClausesOfKind<OMPNowaitClause>(); | ||
| CGM.getOpenMPRuntime().emitTaskwaitCall(*this, S.getBeginLoc(), Data); | ||
| } | ||
|
|
||
| void CodeGenFunction::EmitOMPTaskwaitDirective(const OMPTaskwaitDirective &S) { | ||
| OMPTaskDataTy Data; | ||
| // Build list of dependences | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think these functions should not be needed, if something is required, it should be build in Sema and stored in AST. If you need to replace some AST values by some LVM IR values, use OpaqueValue nodes, which can be replaced in codegen by special RAIIs.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please clarify your comment based on my following points?
transformCallInStmt. InreplaceWithNewTraitsOrDirectCallI am trying to set the call based on BinaryExpr under the dispatch directive.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with these transformations is that sometimes they do not work as expected and miss some cases. Especially it happens with C++, which has lots of idioms like classes (especially inheritance), templates, implicit conversions (functions), etc. All these tree traversal techniques tend to be not fully correct in many cases. That;s why the preferred way to build helper expressions is doing it in Sema, because Sema knows how to handle all these cases correctly, but codegen does not. That's why I'm asking about compatibility with C++, it has lots of corner cases, and it would be good somehow to handle them in this patch rather than fixing it later