[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: clang/include/clang/AST/OpenMPClause.h:6682 + /// The outermost level of selector sets. + llvm::SmallVector Sets; + jdoerfert wrote: > rnk wrote: > > This is not a good

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: clang/include/clang/AST/OpenMPClause.h:6682 + /// The outermost level of selector sets. + llvm::SmallVector Sets; + rnk wrote: > This is not a good data structure choice.

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

rnk added inline comments. Comment at: clang/include/clang/AST/OpenMPClause.h:6682 + /// The outermost level of selector sets. + llvm::SmallVector Sets; + This is not a good data structure choice. You have three levels of small vector nesting, so sizeof

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

fhahn added a comment. It looks like this may cause ASan failures http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/38749/steps/check-clang%20asan/logs/stdio It would be great if you could take a look and possibly revert the change if it takes some time to investigate.

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

aaron.ballman accepted this revision. aaron.ballman added a comment. LGTM aside from some small nits. Comment at: clang/include/clang/Basic/Attr.td:186-192 +// an OMPTraitInfo object. The structure of an OMPTraitInfo object is a +// tree as defined below: +// +//

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

jdoerfert marked 11 inline comments as done. jdoerfert added inline comments. Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:367 } else { -llvm_unreachable("Unknown SimpleArgument type!"); +OS << "if (SA->get" << getUpperName() << "())\n "; +

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

jdoerfert added inline comments. Comment at: clang/lib/AST/OpenMPClause.cpp:1723 +llvm::APInt CondVal = +Selector.ScoreOrCondition->EvaluateKnownConstInt(ASTCtx); +if (CondVal.isNullValue()) aaron.ballman wrote: > jdoerfert wrote: > >

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

aaron.ballman added inline comments. Comment at: clang/include/clang/Basic/Attr.td:185 +// An argument of type \p type with name \p name. +class GenericPointerArgument : Argument { + string Type = type; jdoerfert wrote: > aaron.ballman wrote: > > The name seems

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

jdoerfert marked 14 inline comments as done. jdoerfert added inline comments. Comment at: clang/include/clang/Basic/Attr.td:185 +// An argument of type \p type with name \p name. +class GenericPointerArgument : Argument { + string Type = type; aaron.ballman

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

aaron.ballman added a comment. In D71830#1871838 , @JonChesterfield wrote: > Procedural note - adding someone as a blocking reviewer to someone else's > patch isn't great. What if the new reviewer never gets around to looking at > the patch? > > I've

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

JonChesterfield added a comment. Procedural note - adding someone as a blocking reviewer to someone else's patch isn't great. What if the new reviewer never gets around to looking at the patch? I've adjusted that to non-blocking, but feel free to leave a comment if I've missed something.

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

kiranchandramohan accepted this revision. kiranchandramohan added a comment. This revision is now accepted and ready to land. LGTM. You can wait for a day in case other reviewers have comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:6575 +template <> void

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

jdoerfert marked 4 inline comments as done. jdoerfert added a comment. @kiranchandramohan @JonChesterfield I will address the comments just made, if you think this is otherwise fine it would be good if you accept the patch (it's been around for weeks so it's not really people didn't have a

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

kiranchandramohan added inline comments. Comment at: clang/lib/Serialization/ASTWriter.cpp:6575 +template <> void ASTRecordWriter::writeUserType(OMPTraitInfo *TI) { + writeUInt32(TI->Sets.size()); Had to also move this up in the file to avoid an instantiation

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

kiranchandramohan added a comment. Looks OK to me. Couple of comments inline. Comment at: clang/include/clang/Serialization/ASTRecordReader.h:272 + + template <> OpenMPTraitInfo *readUserType() { return readOpenMPTraitInfo(); } + jdoerfert wrote: > jdoerfert

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

jdoerfert added a comment. @kiranchandramohan Do you have more input on this one? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71830/new/ https://reviews.llvm.org/D71830 ___ cfe-commits mailing list

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: clang/include/clang/Serialization/ASTRecordReader.h:272 + + template <> OpenMPTraitInfo *readUserType() { return readOpenMPTraitInfo(); } + jdoerfert wrote: >

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: clang/include/clang/Serialization/ASTRecordReader.h:272 + + template <> OpenMPTraitInfo *readUserType() { return readOpenMPTraitInfo(); } + kiranchandramohan wrote: >

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

kiranchandramohan added inline comments. Comment at: clang/include/clang/Serialization/ASTRecordReader.h:272 + + template <> OpenMPTraitInfo *readUserType() { return readOpenMPTraitInfo(); } + jdoerfert wrote: > kiranchandramohan wrote: > > jdoerfert wrote: > >

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

jdoerfert marked an inline comment as done. jdoerfert added inline comments. Comment at: clang/include/clang/Serialization/ASTRecordReader.h:272 + + template <> OpenMPTraitInfo *readUserType() { return readOpenMPTraitInfo(); } + kiranchandramohan wrote: >

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

kiranchandramohan added inline comments. Comment at: clang/include/clang/Serialization/ASTRecordReader.h:272 + + template <> OpenMPTraitInfo *readUserType() { return readOpenMPTraitInfo(); } + jdoerfert wrote: > kiranchandramohan wrote: > > Compiler throws up

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62417 tests passed, 0 failed and 845 were skipped. {icon check-circle color=green} clang-tidy: pass. {icon times-circle color=red} clang-format: fail. Please format your changes with clang-format by running

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

merge_guards_bot added a comment. {icon check-circle color=green} Unit tests: pass. 62417 tests passed, 0 failed and 845 were skipped. {icon times-circle color=red} clang-tidy: fail. clang-tidy found 0 errors and 6 warnings

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

jdoerfert marked 4 inline comments as done. jdoerfert added a comment. In D71830#1854989 , @kiranchandramohan wrote: > Had a quick look today. Will spend some more time tomorrow. > Please see a few comments inline. Thanks! I'll rebase asap to address

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

kiranchandramohan added a comment. Had a quick look today. Will spend some more time tomorrow. Please see a few comments inline. Comment at: clang/include/clang/AST/OpenMPClause.h:6429 +/// and an ordered collection of properties. +struct OpenMPTraitInfo { + struct

[PATCH] D71830: [OpenMP][Part 2] Use reusable OpenMP context/traits handling

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