Fix checkpoint loop pragma handling in hessian flows (#1736)#1756
Fix checkpoint loop pragma handling in hessian flows (#1736)#1756fatfat123-arch wants to merge 1 commit intovgvassilev:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Please address the relevant clang-tidy issues along with valgrind failures. |
Hi @vgvassilev ! I pushed a follow-up commit on this PR: It addresses the requested clang-tidy/valgrind-related items in
I re-ran local checks and they pass:
Could you please approve/re-run the PR workflows for this fork branch so the updated CI checks can execute? |
tools/ClangPlugin.cpp
Outdated
| continue; | ||
|
|
||
| clang::SourceLocation AfterPragma = | ||
| clang::Lexer::getLocForEndOfToken(pair.first, 0, SM, LangOpts); |
There was a problem hiding this comment.
There must be a better way than using a lexer for this. How does clang do it?
There was a problem hiding this comment.
Thank you for the feedback !
I used the lexer lookahead as a temporary minimal fix to validate the root cause quickly, but I agree we should align with Clang’s approach and avoid raw-token peeking here.
Another plan would be to bind #pragma clad checkpoint loop to the immediately following parsed statement and then treat it as valid only for ForStmt / WhileStmt / DoStmt, while keeping diagnoseUnusedPragma for unattached pragma.
Would you recommend implementing this through a dedicated pragma-handler/parser path, or through request-time AST association in Clad? I’d appreciate your guidance on which direction you suggest would be best.
There was a problem hiding this comment.
My preference is whatever is done in clang and looks like a lean and simple enough implementation.
There was a problem hiding this comment.
I updated it ! Now #pragma clad checkpoint loop is checked against the next parsed statement and accepted only for for / while / do; otherwise it emits the same diagnostic.
Commit: 4d7b61e
There was a problem hiding this comment.
Can you give the relevant clang code that does something similar?
There was a problem hiding this comment.
The relevant clang code for the similar behavior is:
clang/lib/Parse/ParseStmt.cpp:tok::annot_pragma_loop_hint->ParsePragmaLoopHint(...)clang/lib/Parse/ParseStmt.cpp:ParsePragmaLoopHint(...)parses the pragma, parses the immediately following statement, and prepends pragma attrs to that statement.clang/lib/Sema/SemaStmtAttr.cpp:handleLoopHintAttr(...)accepts only loop statements and otherwise emitsdiag::err_pragma_loop_precedes_nonloop.clang/include/clang/Basic/DiagnosticSemaKinds.td:err_pragma_loop_precedes_nonloop.
|
clang-tidy review says "All clean, LGTM! 👍" |
fatfat123-arch
force-pushed
the
fix-1736-checkpoint-loop
branch
from
4eb3bf7 to
59eee5f
Compare
|
@vgvassilev quick update: I pushed the final fix in commit |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Quick clarification request: CI is green on this PR, but issue #1736 is already closed. Should this PR still be merged, or should I close it as superseded? @vgvassilev |
fatfat123-arch
force-pushed
the
fix-1736-checkpoint-loop
branch
from
59eee5f to
658cae6
Compare
2 participants
Fixes false-positive
#pragma clad checkpoint loopdiagnostics in hessian-related flows.test/Regressions/issue-1736.cpptools/ClangPlugin.cppso dangling checkpoint pragma diagnostics are only emitted when appropriate in hessian/relevant flows, avoiding false positives during hessian schedulingPassed locally:
ninja check-clad-regressionsninja check-clad-diagnosticslit -sv build/testlit -sv build/test/Regressions/issue-1736.cpp