Flang: Add support for Cray pointers in OpenMP constructs #1
+45
−10
Conversation
This file contains 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
…sharing attributes
Issue description:
When DEFAULT DSA is NONE, all data refs must be listed in one of the data sharing clause.
When a Cray pointer is listed in one of the data sharing clause, Cray pointee can be used in parallel region.
This is valid as per standard.
"Cray pointees have the same data-sharing attribute as the storage with which their Cray pointers are associated."
Currently compiler crashes for default(none).
Also current semantic checks incorrectly updates the symbol flags related to Craypointee symbols.
due to this incorrect updation, compiler crashes when default(private) and default(firstprivate) is specified.
Solution:
Added an additional check to skip updation of symbol flags related to Craypointee symbols.
This also prevents from checking cray pointee when DEFAULT DSA is NONE.
This patch has code changes and a test case.
WalkthroughThe pull request introduces modifications to the Flang compiler's handling of Cray pointers in OpenMP constructs. A change in the Changes
Sequence DiagramsequenceDiagram
participant Compiler
participant OmpAttributeVisitor
participant Symbol
Compiler->>OmpAttributeVisitor: Process OpenMP Attributes
OmpAttributeVisitor->>Symbol: Check Symbol Type
alt Is CrayPointee
OmpAttributeVisitor-->>Compiler: Early Return
else Not CrayPointee
OmpAttributeVisitor->>Symbol: Continue Processing
end
Poem
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai help |
CodeRabbit Commands (Invoked using PR comments)
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
flang/test/Semantics/OpenMP/cray-pointer-usage.f90 (2)
12-26: Consider enhancing test coverage and verification.While the parallel regions effectively test different data-sharing combinations, consider these improvements:
- Add explicit value verification instead of just printing
- Add synchronization between parallel regions
- Include a test case for threadprivate
Example addition for threadprivate:
!$omp threadprivate(ivar) !$omp parallel num_threads(2) print *, var(1) !$omp end parallel
1-29: Consider structured test result verification.While the test effectively covers various OpenMP data-sharing attributes with Cray pointers, consider implementing a more structured verification approach:
- Add expected results as comments
- Implement result validation instead of print statements
- Consider using OpenMP critical sections for deterministic output
Example structured verification:
real*8 :: expected_value = 42.0 logical :: test_passed = .true. !$omp parallel num_threads(2) default(none) shared(ivar, test_passed, expected_value) !$omp critical if (var(1) /= expected_value) then test_passed = .false. endif !$omp end critical !$omp end parallel
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
flang/lib/Semantics/resolve-directives.cpp(1 hunks)flang/test/Semantics/OpenMP/cray-pointer-usage.f90(1 hunks)
🔇 Additional comments (3)
flang/lib/Semantics/resolve-directives.cpp (1)
2245-2260: LGTM! The special handling for Cray pointers is correct.The changes properly handle Cray pointers by skipping flag updates when a symbol has the CrayPointee flag set, while still preserving the DEFAULT(NONE) clause validation. This prevents unnecessary symbol flag modifications for Cray pointers in OpenMP constructs.
flang/test/Semantics/OpenMP/cray-pointer-usage.f90 (2)
1-8: LGTM! Well-structured test setup.The test configuration and variable declarations are correct. The use of
test_errors.pywith-fopenmpflag ensures proper OpenMP directive validation.
9-11: LGTM! Correct pointer initialization sequence.The initialization of the pointee array and Cray pointer setup using
loc()follows the standard pattern.
kiranktp
pushed a commit
that referenced
this pull request
Feb 13, 2025
…StrictPackMatch field (llvm#126215) This addresses the MSAN failure reported in llvm#125791 (comment): ``` ==5633==WARNING: MemorySanitizer: use-of-uninitialized-value #0 in clang::ASTNodeImporter::CallOverloadedCreateFun<clang::ClassTemplateSpecializationDecl>::operator() #1 in bool clang::ASTNodeImporter::GetImportedOrCreateSpecialDecl<...> ... ``` The ASTImporter reads `D->hasStrictPackMatch()` and forwards it to the constructor of the destination `ClassTemplateSpecializationDecl`. But if `D` is a decl that LLDB created from debug-info, it would've been created using `ClassTemplateSpecializationDecl::CreateDeserialized`, which doesn't initialize the `StrictPackMatch` field. This patch just initializes the field to a fixed value of `false`, to preserve previous behaviour and avoid the use-of-uninitialized-value. An alternative would be to always initialize it in the `ClassTemplateSpecializationDecl` constructor, but there were reservations about providing a default value for it because it might lead to hard-to-diagnose problems down the line.
kiranktp
pushed a commit
that referenced
this pull request
Feb 13, 2025
We can't guaranty that underlying string is 0-terminated and [String.size()] is even in the same allocation. https://lab.llvm.org/buildbot/#/builders/94/builds/4152/steps/17/logs/stdio ``` ==c-index-test==1846256==WARNING: MemorySanitizer: use-of-uninitialized-value #0 in clang::cxstring::createRef(llvm::StringRef) llvm-project/clang/tools/libclang/CXString.cpp:96:36 #1 in DumpCXCommentInternal llvm-project/clang/tools/c-index-test/c-index-test.c:521:39 llvm#2 in DumpCXCommentInternal llvm-project/clang/tools/c-index-test/c-index-test.c:674:7 llvm#3 in DumpCXCommentInternal llvm-project/clang/tools/c-index-test/c-index-test.c:674:7 llvm#4 in DumpCXComment llvm-project/clang/tools/c-index-test/c-index-test.c:685:3 llvm#5 in PrintCursorComments llvm-project/clang/tools/c-index-test/c-index-test.c:768:7 Memory was marked as uninitialized #0 in __msan_allocated_memory llvm-project/compiler-rt/lib/msan/msan_interceptors.cpp:1023:5 #1 in Allocate llvm-project/llvm/include/llvm/Support/Allocator.h:172:7 llvm#2 in Allocate llvm-project/llvm/include/llvm/Support/Allocator.h:216:12 llvm#3 in Allocate llvm-project/llvm/include/llvm/Support/AllocatorBase.h:53:43 llvm#4 in Allocate<char> llvm-project/llvm/include/llvm/Support/AllocatorBase.h:76:29 llvm#5 in convertCodePointToUTF8 llvm-project/clang/lib/AST/CommentLexer.cpp:42:30 llvm#6 in clang::comments::Lexer::resolveHTMLDecimalCharacterReference(llvm::StringRef) const llvm-project/clang/lib/AST/CommentLexer.cpp:76:10 llvm#7 in clang::comments::Lexer::lexHTMLCharacterReference(clang::comments::Token&) llvm-project/clang/lib/AST/CommentLexer.cpp:615:16 llvm#8 in consumeToken llvm-project/clang/include/clang/AST/CommentParser.h:62:9 llvm#9 in clang::comments::Parser::parseParagraphOrBlockCommand() llvm-project/clang/lib/AST/CommentParser.cpp llvm#10 in clang::comments::Parser::parseFullComment() llvm-project/clang/lib/AST/CommentParser.cpp:925:22 llvm#11 in clang::RawComment::parse(clang::ASTContext const&, clang::Preprocessor const*, clang::Decl const*) const llvm-project/clang/lib/AST/RawCommentList.cpp:221:12 llvm#12 in clang::ASTContext::getCommentForDecl(clang::Decl const*, clang::Preprocessor const*) const llvm-project/clang/lib/AST/ASTContext.cpp:714:35 llvm#13 in clang_Cursor_getParsedComment llvm-project/clang/tools/libclang/CXComment.cpp:36:35 llvm#14 in PrintCursorComments llvm-project/clang/tools/c-index-test/c-index-test.c:756:25 ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
@coderabbitai
Summary by CodeRabbit
New Features
Tests
test_crayptrdemonstrating Cray pointer behavior with different OpenMP sharing attributes.Bug Fixes