[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. I think you're referring to stuff like `OpenMPCaptureLevel` in `ScopeInfo.h`. I wrote those changes for this patch first, as can be seen in phabricator history. I needed them for the other patch too, and I thought we were going to end up applying that patch first, so I

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny updated this revision to Diff 215298. jdenny added a comment. Rebase. Add more tests, as requested at D66247#1630441 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65835/new/ https://reviews.llvm.org/D65835 Files: clang/include/clang

[PATCH] D66247: [OpenMP] Fix target map for unused variables

jdenny added a comment. In D66247#1630352 , @ABataev wrote: > Yes, just realized that, defaultmap does not affect explicit firstprivates. > Then just check `map(a) firstprivate(a)` for `int128` type. Check that it > still has `tofrom` mapping. If so, the

[PATCH] D66247: [OpenMP] Fix target map for unused variables

jdenny added a comment. In D66247#1630321 , @ABataev wrote: > Try `map(a) firstprivate(a) defaultmap(scalar:tofrom)`, where `a` is `int`, > for example. The variable must be mapped as `tofrom` in this case but, most > probably, will be mapped as `to`.

[PATCH] D66247: [OpenMP] Fix target map for unused variables

jdenny added a comment. In D66247#1630262 , @ABataev wrote: > In D66247#1630245 , @jdenny wrote: > > > In D66247#1630196 , @ABataev wrote: > > > > > Do we really need to map

[PATCH] D66247: [OpenMP] Fix target map for unused variables

jdenny added a comment. In D66247#1630196 , @ABataev wrote: > Do we really need to map such variables? According to standard, "The map > clause specifies how an original list item is mapped from the current task’s > data environment to a corresponding li

[PATCH] D66247: [OpenMP] Fix target map for unused variables

jdenny created this revision. jdenny added reviewers: ABataev, jdoerfert, hfinkel, kkwli0. Herald added a subscriber: guansong. Herald added a project: clang. Without this patch, each of the following `map` clauses doesn't map its variable into the target region because the variable is unused in

[PATCH] D61466: [Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug

jdenny added a comment. In D61466#1613571 , @jdenny wrote: > Note the reproducer in the implementation's FIXME. The other FIXME points > there, so I figure it's best not to duplicate the note. @jkorous, is this change sufficient? Thanks. CHANGES SIN

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. In D65835#1624659 , @ABataev wrote: > In D65835#1624629 , @jdenny wrote: > > > In D65835#1624624 , @ABataev wrote: > > > > > In D65835#1624619

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. In D65835#1624624 , @ABataev wrote: > In D65835#1624619 , @jdenny wrote: > > > In D65835#1624617 , @ABataev wrote: > > > > > Target teams private map

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. In D65835#1624617 , @ABataev wrote: > Target teams private map will produce extra private in target context, other > constructs either. Here's what I tried: int a; #pragma omp target teams private(a) map(a) ; The same c

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny added a comment. In D65835#1624615 , @ABataev wrote: > This is wrong. It affects all possible combinations and not only fof scalar > types, all of them are affected. Are you saying the patch isn't sufficient because other types need to be fixed

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny updated this revision to Diff 214559. jdenny marked 3 inline comments as done. jdenny edited the summary of this revision. jdenny set the repository for this revision to rG LLVM Github Monorepo. jdenny added a comment. In D65835#1624477 , @ABataev wr

[PATCH] D65835: [OpenMP] Permit map with DSA on combined directive

jdenny updated this revision to Diff 214542. jdenny retitled this revision from "[OpenMP] Fix map/is_device_ptr with DSA on combined directive" to "[OpenMP] Permit map with DSA on combined directive". jdenny edited the summary of this revision. jdenny added a comment. I made the following changes

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

jdenny added a comment. In D65835#1623858 , @ABataev wrote: > In D65835#1623854 , @jdenny wrote: > > > In D65835#1623814 , @hfinkel wrote: > > > > > In D65835#1623772

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

jdenny added a comment. In D65835#1623814 , @hfinkel wrote: > In D65835#1623772 , @ABataev wrote: > > > In D65835#1623756 , @kkwli0 wrote: > > > > > In D65835#1622042

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

jdenny added a comment. In D65835#1621202 , @ABataev wrote: > >> See 2.19.7 Data-Mapping Attribute Rules, Clauses, and Directives > > > > I looked again. I'm still not finding any text in that section that > > implies is_device_ptr follows the same rest

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

jdenny marked 2 inline comments as done. jdenny added a comment. In D65835#1619560 , @ABataev wrote: > In D65835#1619549 , @jdenny wrote: > > > In D65835#1619526 , @ABataev w

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

jdenny added a comment. In D65835#1619526 , @ABataev wrote: > > Maybe, but I haven't found any statement in either version that states that > > map restrictions apply to is_device_ptr. > > `is_device_ptr` is a kind of mapping clause. I assume we can exten

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

jdenny marked an inline comment as done. jdenny added a comment. In D65835#1618865 , @ABataev wrote: > `is_device_ptr` can be considered as a kind of mapping clause (see 2.19.7 > Data-Mapping Attribute Rules, Clauses, and Directives), so, I assume, clan

[PATCH] D65835: [OpenMP] Fix map/is_device_ptr with DSA on combined directive

jdenny created this revision. jdenny added a reviewer: ABataev. Herald added a subscriber: guansong. Herald added a reviewer: jdoerfert. Herald added a project: clang. For `map`, the following restriction changed in OpenMP 5.0: - OpenMP 4.5 [2.15.5.1, Restrictions]: "A list item cannot appear in

[PATCH] D61466: [Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug

jdenny updated this revision to Diff 213197. jdenny added a comment. Note the reproducer in the implementation's FIXME. The other FIXME points there, so I figure it's best not to duplicate the note. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61466/new/ https://reviews.llvm.org/D614

[PATCH] D61466: [Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug

jdenny added a comment. In D61466#1602917 , @jkorous wrote: > Hi @jdenny, thanks for the warning! Hi. Thanks for the review. > I think having the test disabled is fine - the main upside I see is that we > won't check it fails over and over on our CI s

[PATCH] D61466: [Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug

jdenny added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61466/new/ https://reviews.llvm.org/D61466 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61466: [Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug

jdenny added a comment. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61466/new/ https://reviews.llvm.org/D61466 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D61466: [Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug

jdenny updated this revision to Diff 208795. jdenny retitled this revision from "[Rewrite][NFC] Add FIXME about RemoveLineIfEmpty" to "[Rewrite][NFC] Add FIXMEs and tests for RemoveLineIfEmpty bug". jdenny edited the summary of this revision. jdenny added a comment. Rebase, and add a unit test t

[PATCH] D61467: [Rewrite] Extend to further accept CharSourceRange

jdenny added inline comments. Comment at: cfe/trunk/unittests/Rewrite/CMakeLists.txt:12 clangRewrite + clangTooling ) jdenny wrote: > thakis wrote: > > This makes RewriteTests depend on clangTooling, and in follow-ups on > > clangFrontend and clangSeriali

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1596 << Ty << E->getSourceRange(); + if (Ty->isRealFloatingType()) { +llvm::APFloatBase::Semantics Sem = llvm::APFloatBase::SemanticsToEnum( ABataev wrote: > jdenny wrote: > > A

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny abandoned this revision. jdenny added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1596 << Ty << E->getSourceRange(); + if (Ty->isRealFloatingType()) { +llvm::APFloatBase::Semantics Sem = llvm::APFloatBase::SemanticsToEnum( ABat

[PATCH] D61467: [Rewrite] Extend to further accept CharSourceRange

jdenny added inline comments. Comment at: cfe/trunk/unittests/Rewrite/CMakeLists.txt:12 clangRewrite + clangTooling ) thakis wrote: > This makes RewriteTests depend on clangTooling, and in follow-ups on > clangFrontend and clangSerialization. Rewrite used

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1596 << Ty << E->getSourceRange(); + if (Ty->isRealFloatingType()) { +llvm::APFloatBase::Semantics Sem = llvm::APFloatBase::SemanticsToEnum( ABataev wrote: > jdenny wrote: > > A

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1596 << Ty << E->getSourceRange(); + if (Ty->isRealFloatingType()) { +llvm::APFloatBase::Semantics Sem = llvm::APFloatBase::SemanticsToEnum( ABataev wrote: > jdenny wrote: > > A

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1596 << Ty << E->getSourceRange(); + if (Ty->isRealFloatingType()) { +llvm::APFloatBase::Semantics Sem = llvm::APFloatBase::SemanticsToEnum( ABataev wrote: > jdenny wrote: > > A

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1596 << Ty << E->getSourceRange(); + if (Ty->isRealFloatingType()) { +llvm::APFloatBase::Semantics Sem = llvm::APFloatBase::SemanticsToEnum( ABataev wrote: > Why do we need all

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny updated this revision to Diff 208286. jdenny added a comment. This incorporates all the improvements I think we've discussed so far: - In diagnostics, distinguish between unsupported types and non-equivalent types. - Don't treat `__float128` and `long double` as equivalent. - Compare exac

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594 + !Context.getTargetInfo().hasFloat128Type() && + Context.getTargetInfo().getLongDoubleWidth() != 128) || (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 && ABa

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594 + !Context.getTargetInfo().hasFloat128Type() && + Context.getTargetInfo().getLongDoubleWidth() != 128) || (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 && ABa

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594 + !Context.getTargetInfo().hasFloat128Type() && + Context.getTargetInfo().getLongDoubleWidth() != 128) || (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 && ABa

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594 + !Context.getTargetInfo().hasFloat128Type() && + Context.getTargetInfo().getLongDoubleWidth() != 128) || (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 && ABa

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594 + !Context.getTargetInfo().hasFloat128Type() && + Context.getTargetInfo().getLongDoubleWidth() != 128) || (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 && ABa

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594 + !Context.getTargetInfo().hasFloat128Type() && + Context.getTargetInfo().getLongDoubleWidth() != 128) || (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 && ABa

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:1594 + !Context.getTargetInfo().hasFloat128Type() && + Context.getTargetInfo().getLongDoubleWidth() != 128) || (Ty->isIntegerType() && Context.getTypeSize(Ty) == 128 && ABa

[PATCH] D64289: [OpenMP] Fix 128-bit long double support on target

jdenny created this revision. jdenny added a reviewer: ABataev. Herald added subscribers: jdoerfert, guansong. Herald added a project: clang. For example, without this fix, when the host is x86_64, long double is sometimes rejected when offloading to x86_64: $ cat test.c int main() { long

[PATCH] D61467: [Rewrite] Extend to further accept CharSourceRange

This revision was automatically updated to reflect the committed changes. Closed by commit rL365258: [Rewrite] Extend to further accept CharSourceRange (authored by jdenny, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://re

[PATCH] D61467: [Rewrite] Extend to further accept CharSourceRange

jdenny updated this revision to Diff 208247. jdenny added a comment. @jdoerfert Thanks for your reply. I added some comments to the tests. Let me know whether that's what you're looking for. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61467/new/ https://reviews.llvm.org/D61467 Fil

[PATCH] D61467: [Rewrite] Extend to further accept CharSourceRange

jdenny updated this revision to Diff 207301. jdenny added a comment. Rebased, fixed a comment, and applied a clang-format suggestion. Ping. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61467/new/ https://reviews.llvm.org/D61467 Files: clang/include/clang/Rewrite/Core/Rewriter.h c

[PATCH] D61467: [Rewrite] Extend to further accept CharSourceRange

jdenny added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61467/new/ https://reviews.llvm.org/D61467 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

[PATCH] D61509: [OpenMP] Set pragma start loc to `#pragma` loc

This revision was automatically updated to reflect the committed changes. Closed by commit rL361867: [OpenMP] Set pragma start loc to `#pragma` loc (authored by jdenny, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://review

[PATCH] D61467: [Rewrite] Extend to further accept CharSourceRange

jdenny added a comment. Ping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61467/new/ https://reviews.llvm.org/D61467 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi

[PATCH] D61509: [OpenMP] Set pragma start loc to `#pragma` loc

jdenny added a comment. In D61509#1512601 , @Meinersbur wrote: > > I would think different people would want to review different pragmas, so > > separate patches would be better, but I'm happy to be corrected as I > > haven't explored who owns what here.

[PATCH] D61509: [OpenMP] Set pragma start loc to `#pragma` loc

jdenny added a comment. In D61509#1512570 , @aaron.ballman wrote: > These changes LGTM! Thanks, and thanks to everyone who chimed in. > I'd say give it a few days in case other reviewers would like to chime in. Sure. Maybe this weekend or so. CHANG

[PATCH] D61509: [OpenMP] Set pragma start loc to `#pragma` loc

jdenny added a comment. In D61509#1512488 , @aaron.ballman wrote: > In D61509#1512090 , @jdenny wrote: > > > Now that D61643 is pushed, we're back to > > this patch. My recollect

[PATCH] D61509: [OpenMP] Set pragma start loc to `#pragma` loc

jdenny added a comment. In D61509#1512321 , @Meinersbur wrote: > In D61509#1512311 , @jdenny wrote: > > > 2. I too think it likely makes sense to adjust them all eventually. But do > > people think it's important

[PATCH] D61509: [OpenMP] Set pragma start loc to `#pragma` loc

jdenny added a comment. In D61509#1512294 , @ABataev wrote: > In D61509#1512293 , @Meinersbur > wrote: > > > 1. Is there a diagnostic that would point to the `omp` token? As much as I > > like complete info (such

[PATCH] D61509: [OpenMP] Set pragma start loc to `#pragma` loc

jdenny added a subscriber: Meinersbur. jdenny added a comment. Now that D61643 is pushed, we're back to this patch. My recollection is there are two remaining issues: 1. Should we store both the `#pragma` location and the `omp` location in the AST, or is it fi

[PATCH] D61643: [PragmaHandler] Expose `#pragma` location

This revision was automatically updated to reflect the committed changes. Closed by commit rC361335: [PragmaHandler] Expose `#pragma` location (authored by jdenny, committed by ). Changed prior to commit: https://reviews.llvm.org/D61643?vs=198489&id=200608#toc Repository: rC Clang CHANGES S

[PATCH] D61643: [PragmaHandler] Expose `#pragma` location

jdenny added a comment. In D61643#1510815 , @Meinersbur wrote: > +1 > > Such a solution also came up in https://bugs.llvm.org/show_bug.cgi?id=41514#c1 Cool. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61

[PATCH] D61643: [PragmaHandler][NFC] Expose `#pragma` location

jdenny marked an inline comment as done. jdenny added inline comments. Comment at: clang/docs/ClangPlugins.rst:58 ExamplePragmaHandler() : PragmaHandler("example_pragma") { } -void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer, +void HandlePragma(Pre

[PATCH] D61643: [PragmaHandler][NFC] Expose `#pragma` location

jdenny marked an inline comment as done. jdenny added inline comments. Comment at: clang/docs/ClangPlugins.rst:58 ExamplePragmaHandler() : PragmaHandler("example_pragma") { } -void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer, +void HandlePragma(Pre

[PATCH] D61643: [PragmaHandler][NFC] Expose `#pragma` location

jdenny marked an inline comment as done. jdenny added inline comments. Comment at: clang/docs/ClangPlugins.rst:58 ExamplePragmaHandler() : PragmaHandler("example_pragma") { } -void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer, +void HandlePragma(Pre

[PATCH] D61467: [Rewrite] Extend to further accept CharSourceRange

jdenny added inline comments. Comment at: clang/unittests/Rewrite/RewriterTest.cpp:1 +//===- unittests/Rewrite/RewriteTest.cpp - RewriteBuffer tests ===// +// Oops. Need to change the description here. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D61467: [Rewrite] Extend to further accept CharSourceRange

jdenny added a comment. Ping. This seems like a straight-forward extension to the Rewriter API. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61467/new/ https://reviews.llvm.org/D61467 ___ cfe-commits

[PATCH] D61643: [PragmaHandler][NFC] Expose `#pragma` location

jdenny marked an inline comment as done. jdenny added inline comments. Comment at: clang/docs/ClangPlugins.rst:58 ExamplePragmaHandler() : PragmaHandler("example_pragma") { } -void HandlePragma(Preprocessor &PP, PragmaIntroducerKind Introducer, +void HandlePragma(Pre

[PATCH] D61509: [OpenMP] Set pragma start loc to `#pragma` loc

jdenny updated this revision to Diff 198491. jdenny retitled this revision from "[PragmaHandler] Expose `#pragma` location" to "[OpenMP] Set pragma start loc to `#pragma` loc". jdenny edited the summary of this revision. jdenny added a comment. As requested, replace this patch with just the OpenM

[PATCH] D61643: [PragmaHandler][NFC] Expose `#pragma` location

jdenny created this revision. jdenny added reviewers: ABataev, lebedev.ri. Herald added a subscriber: jdoerfert. Herald added a project: clang. Currently, a pragma AST node's recorded location starts at the namespace token (such as `omp` in the case of OpenMP) after the `#pragma` token, and the

[PATCH] D61509: [PragmaHandler] Expose `#pragma` location

jdenny added a comment. In D61509#1493729 , @lebedev.ri wrote: > I don't see why this differential can't be updated to only contain the > remaining part > of the diff (for the actual OpenMP change), after splitting the NFC > refactoring part. OK Rep

[PATCH] D61509: [PragmaHandler] Expose `#pragma` location

jdenny added a comment. In D61509#1493718 , @lebedev.ri wrote: > In D61509#1493714 , @jdenny wrote: > > > In D61509#1493703 , @lebedev.ri > > wrote: > > > > > It would have

[PATCH] D61509: [PragmaHandler] Expose `#pragma` location

jdenny added a comment. In D61509#1493703 , @lebedev.ri wrote: > It would have been better to submit this refactor as a new patch.. Sorry, I didn't realize that was the norm. I can do that now if it would help. I can also revert changes to this patch

[PATCH] D61509: [PragmaHandler] Expose `#pragma` location

jdenny updated this revision to Diff 198488. jdenny retitled this revision from "[PragmaHandler][OpenMP] Expose `#pragma` location" to "[PragmaHandler] Expose `#pragma` location". jdenny edited the summary of this revision. jdenny set the repository for this revision to rG LLVM Github Monorepo. jd

[PATCH] D61566: Fix for bug 41747: AST Printer doesn't print nested name specifier for out of scope record definitions

jdenny added reviewers: rsmith, aaron.ballman. jdenny accepted this revision. jdenny added a comment. This revision is now accepted and ready to land. LGTM except for nits in the tests. I'm not close to C++ support in Clang, so please give other reviewers a few days to comment just in case. I'v

[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

jdenny added a comment. Thanks for everyone's responses so far. At this point, I'm going to follow the uncontroversial suggestions from @lebedev.ri: create a `struct PragmaIntroducer`, and split the OpenMP work into a second patch. That will hopefully make it easier for @rsmith or others to c

[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

jdenny added a comment. In D61509#1491898 , @ABataev wrote: > Again, I don't see a single point why would we need this. I don't think there > is a big difference between the location of pragma keyword and `omp` keyword. @lebedev.ri : You said you're not

[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

jdenny added a comment. In D61509#1491537 , @lebedev.ri wrote: > In D61509#1491397 , @jdenny wrote: > > > In D61509#1491209 , @ABataev wrote: > > > > > In D61509#1491204

[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

jdenny added a comment. In D61509#1491209 , @ABataev wrote: > In D61509#1491204 , @lebedev.ri > wrote: > > > In D61509#1491158 , @jdenny wrote: > > > > > In D61509#1491119

[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

jdenny added a comment. In D61509#1491119 , @lebedev.ri wrote: > I recommend to split this into two parts - changing `PragmaIntroducerKind > Introducer` to > `struct NameMe { PragmaIntroducerKind Kind; SourceLocation Loc};`, and the > actual openmp chan

[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

jdenny updated this revision to Diff 198046. jdenny edited the summary of this revision. jdenny added a comment. Herald added a subscriber: jfb. As discussed, implement OpenMP solution #1 (one-line change in `PragmaOpenMPHandler::HandlePragma` in `ParsePragma.cpp`), and update tests. CHANGES SI

[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

jdenny added a comment. Thanks for explaining. I'll proceed with solution #1. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61509/new/ https://reviews.llvm.org/D61509 ___ cfe-commits mailing list cfe-

[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

jdenny added a comment. In D61509#1489771 , @ABataev wrote: > In D61509#1489761 , @jdenny wrote: > > > In D61509#1489752 , @ABataev wrote: > > > > > If the patch is going to

[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

jdenny added a comment. In D61509#1489752 , @ABataev wrote: > If the patch is going to be accepted, then definitely it must be solution #1. I certainly have no objection to that and will be happy to implement it, but can you help me to understand the ra

[PATCH] D61509: [PragmaHandler][OpenMP] Expose `#pragma` location

jdenny created this revision. jdenny added reviewers: ABataev, rnk, Eugene.Zelenko, akyrtzi, rsmith. Herald added subscribers: jdoerfert, dexonsmith, guansong. Herald added a project: clang. Currently, an OpenMP AST node's recorded location starts at the `omp` token after the `#pragma` token, and

[PATCH] D61467: [Rewrite] Extend to further accept CharSourceRange

jdenny created this revision. jdenny added reviewers: akyrtzi, Eugene.Zelenko, rsmith. Herald added subscribers: dexonsmith, mgorny. Herald added a project: clang. Some Rewrite functions are already overloaded to accept CharSourceRange, and this extends others in the same manner. I'm calling the

[PATCH] D61466: [Rewrite][NFC] Add FIXME about RemoveLineIfEmpty

jdenny created this revision. jdenny added reviewers: akyrtzi, Eugene.Zelenko, rsmith. Herald added a subscriber: dexonsmith. Herald added a project: clang. I'd like to add these comments to warn others of problems I encountered when trying to use RemoveLineIfEmpty. I originally tried to fix the

[PATCH] D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/$target/c++ and include/c++

jdenny added a comment. In D59168#1484754 , @phosek wrote: > It's the `LIBCXX_ABI_VERSION` CMake option, see > https://github.com/llvm/llvm-project/blob/master/libcxx/CMakeLists.txt#L121 Thanks. Comment at: libunwind/CMakeLists.txt:1

[PATCH] D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/$target/c++ and include/c++

jdenny added a comment. In D59168#1480801 , @phosek wrote: > In D59168#1480428 , @jdenny wrote: > > > It seems we have roughly the same situation for their ABIs. That is, for > > .so files, someone noted that libc

[PATCH] D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/clang/ and include/

jdenny added a comment. In D59168#1479939 , @phosek wrote: > In D59168#1474715 , @jdenny wrote: > > > Does the following match what you have in mind? > > > > $prefix/ > > include/ > > c++/ > > v1

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

This revision was automatically updated to reflect the committed changes. Closed by commit rC359012: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types (authored by jdenny, committed by ). Changed prior to commit: https://reviews.llvm.org/D59712?vs=195626&id=196274#toc Repository: rC C

[PATCH] D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/clang/ and include/

jdenny added a comment. Does the following match what you have in mind? $prefix/ include/ c++/ v1/ limits.h ... openmp/ v1/ <-- I don't think openmp has anything like this now omp.h ompt.h ... lib/ c

[PATCH] D60845: [VerifyDiagnosticConsumer] Document -verify= in doxygen

This revision was automatically updated to reflect the committed changes. Closed by commit rC358917: [VerifyDiagnosticConsumer] Document -verify= in doxygen (authored by jdenny, committed by ). Changed prior to commit: https://reviews.llvm.org/D60845?vs=195742&id=196127#toc Repositor

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

jdenny added a comment. Thanks for the reviews! Will push soon. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59712/new/ https://reviews.llvm.org/D59712 ___ cfe-commits mailing list cfe-commits@lists.

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

jdenny added a comment. In D59712#1469392 , @lebedev.ri wrote: > Does this pass `check-all`? `check-all` of stage-2? test-suite? For me, all these tests behave with the current patch. As before, the only subproject I did not build was llgo. Repositor

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

jdenny added a comment. In D59712#1472358 , @hfinkel wrote: > > I've never tried running the other tests you mention, for any patch. I > > thought people normally left those to the bots. Should this patch be > > handled differently? > > We have a lot o

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

jdenny added a comment. In D59712#1472342 , @lebedev.ri wrote: > In D59712#1472318 , @jdenny wrote: > > > For me, check-all's success is not affected by the current patch. I built > > all subprojects except llgo,

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

jdenny added a comment. In D59712#1469760 , @lebedev.ri wrote: > In D59712#1469693 , @jdenny wrote: > > > In D59712#1469392 , @lebedev.ri > > wrote: > > > > > In D59712#1469

[PATCH] D60845: [VerifyDiagnosticConsumer] Document -verify= in doxygen

jdenny added a comment. In D60845#1472291 , @rsmith wrote: > In D60845#1471741 , @jdenny wrote: > > > In D60845#1471002 , @rsmith wrote: > > > > > I've seen a few projects ou

[PATCH] D59168: [runtimes] Move libunwind, libc++abi and libc++ to lib/clang/ and include/

jdenny added a comment. In D59168#1470578 , @phosek wrote: > In D59168#1469186 , @jdenny wrote: > > > > > > I was also thinking about alternative names for the library path, > specifically for headers we use `inclu

[PATCH] D60845: [VerifyDiagnosticConsumer] Document -verify= in doxygen

jdenny added a comment. Thanks for the accepts. In D60845#1470986 , @NoQ wrote: > In D60845#1470980 , @Charusso wrote: > > > I really like live working examples, I hope not just me. Could you link > > https://gith

[PATCH] D60845: [VerifyDiagnosticConsumer] Document -verify= in doxygen

jdenny updated this revision to Diff 195742. jdenny added a comment. Clarify the behavior of multiple -verify options. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60845/new/ https://reviews.llvm.org/D60845 Files: clang/include/clang/Frontend/VerifyDiagnosticConsumer.h Index: clan

[PATCH] D60845: [VerifyDiagnosticConsumer] Document -verify= in doxygen

jdenny created this revision. jdenny added reviewers: NoQ, Charusso, hfinkel, rsmith. Herald added a project: clang. Previously, it was only documented by `-cc1 -help`, so people weren't aware of it, as discussed in D60732 . Repository: rG LLVM Github Monorepo

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

jdenny added a comment. In D59712#1469392 , @lebedev.ri wrote: > In D59712#1469358 , @jdenny wrote: > > > In D59712#1469301 , @lebedev.ri > > wrote: > > > > > In D59712#1469

[PATCH] D59712: [APSInt][OpenMP] Fix isNegative, etc. for unsigned types

jdenny added a comment. In D59712#1469301 , @lebedev.ri wrote: > In D59712#1469295 , @craig.topper > wrote: > > > Wondering if it would be better to assert for asking for the sign of an > > unsigned APSInt. I coul

<    1   2   3   4   5   >