Support for declare target initializers #3
Conversation
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
clang/lib/Sema/SemaDecl.cpp
Outdated
Comment on lines
14478
to
14481
| if (D && D->hasAttr<OMPDeclareTargetDeclAttr>() && isa<VarDecl>(D)) { | ||
| if ((cast<VarDecl>(D))->hasGlobalStorage()) | ||
| ActOnOpenMPImplicitDeclareTarget(D); | ||
| } |
Collaborator
There was a problem hiding this comment.
Suggested change
| if (D && D->hasAttr<OMPDeclareTargetDeclAttr>() && isa<VarDecl>(D)) { | |
| if ((cast<VarDecl>(D))->hasGlobalStorage()) | |
| ActOnOpenMPImplicitDeclareTarget(D); | |
| } | |
| if (D && D->hasAttr<OMPDeclareTargetDeclAttr>() && | |
| isa<VarDecl>(D) && cast<VarDecl>(D))->hasGlobalStorage()) { | |
| ActOnOpenMPImplicitDeclareTarget(D); | |
| } |
clang/lib/Sema/SemaOpenMP.cpp
Outdated
Comment on lines
23099
to
23105
| if (DeclRef) { | ||
| Decl *DeclVar = (Decl *)DeclRef->getDecl(); | ||
| DeclVar->addAttr(TargetDecl->getAttr<OMPDeclareTargetDeclAttr>()); | ||
| return DeclVar; | ||
| } else | ||
| return nullptr; | ||
| } |
Collaborator
There was a problem hiding this comment.
Suggested change
| if (DeclRef) { | |
| Decl *DeclVar = (Decl *)DeclRef->getDecl(); | |
| DeclVar->addAttr(TargetDecl->getAttr<OMPDeclareTargetDeclAttr>()); | |
| return DeclVar; | |
| } else | |
| return nullptr; | |
| } | |
| Decl *DeclVar = nullptr; | |
| if (DeclRef) { | |
| DeclVar = (Decl *)DeclRef->getDecl(); | |
| DeclVar->addAttr(TargetDecl->getAttr<OMPDeclareTargetDeclAttr>()); | |
| } | |
| return DeclVar; |
Also, correct the alignment.
clang/lib/Sema/SemaOpenMP.cpp
Outdated
| TargetDecl = AddOMPDeclareTargetDeclAttr(DeclRef, TargetDecl); | ||
| } | ||
| } else | ||
| break; |
Collaborator
There was a problem hiding this comment.
Please add explicit comments against each break stating that the rest of the elements will not be processed along with the reason.
clang/lib/Sema/SemaOpenMP.cpp
Outdated
| } | ||
| } else | ||
| break; | ||
| } else |
Collaborator
There was a problem hiding this comment.
Always use braces in such complex statements to help readability.
Suggested change
| } else | |
| } else { | |
| ... | |
| } |
sandeepkosuri
approved these changes
May 8, 2023
| if (isa<Expr>(*it)) | ||
| VisitMyExpr(dyn_cast<Expr>(*it)); | ||
| if (isa<DeclRefExpr>(*it)) | ||
| Visit(*it); |
Collaborator
There was a problem hiding this comment.
I am assuming this Visit() calls the MyStmtVisitor::VisitDeclRefExpr() indirectly via overriding, as *it is a DeclRefExpr. Why not call MyStmtVisitor::VisitDeclRefExpr() yourself, if you know that *it is a DeclRefExpr ?
| checkDeclInTargetContext(E->getExprLoc(), E->getSourceRange(), *this, D); | ||
| } | ||
|
|
||
| class MyStmtVisitor final : public ConstStmtVisitor<MyStmtVisitor> { |
Collaborator
There was a problem hiding this comment.
Suggesting not to use My in the class and method names
Comment on lines
+14478
to
+14482
| if (auto *VD = dyn_cast_or_null<VarDecl>(D); | ||
| LangOpts.OpenMP && VD && VD->hasAttr<OMPDeclareTargetDeclAttr>() && | ||
| VD->hasGlobalStorage()) { | ||
| ActOnOpenMPImplicitDeclareTarget(D); | ||
| } |
Collaborator
There was a problem hiding this comment.
Did you run git-clang-format on all your changes ?
| checkDeclIsAllowedInOpenMPTarget(Expr *E, Decl *D, | ||
| SourceLocation IdLoc = SourceLocation()); | ||
|
|
||
| /// Adds OMPDeclareTargetDeclAttr to referenced variables in declare target |
Collaborator
There was a problem hiding this comment.
Instead of explaining what the function does literally, a comment describing the goal of the function would be easier to understand.
RitanyaB
pushed a commit
that referenced
this pull request
Jan 4, 2024
When shl is folded in compare instruction, a miscompilation occurs when the CMP instruction is also sign-extended. For the following IR: %op3 = shl i8 %op2, 3 %tmp3 = icmp eq i8 %tmp2, %op3 It used to generate cmp w8, w9, sxtb #3 which means sign extend w9, shift left by 3, and then compare with the value in w8. However, the original intention of the IR would require `%op2` to first shift left before extending the operands in the comparison operation . Moreover, if sign extension is used instead of zero extension, the sample test would miscompile. This PR creates a fix for the issue, more specifically to not fold the left shift into the CMP instruction, and to create a zero-extended value rather than a sign-extended value.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
No description provided.