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

[Johannes Doerfert via Phabricator via cfe-commits]

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 data structure choice. You have three levels of small 
> > vector nesting, so sizeof OMPTraitInfo is 880 bytes, and then you are 
> > passing it by value in in some of the attribute classes. Are you sure you 
> > wanted to do that?
> I could make them all 0 elements long, reducing the size quite a bit for the 
> common case of very few trait sets/selectors/properties. I could also try to 
> dynamically allocate them once. WDYT?
One way of fixing this is D76173


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

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. You have three levels of small 
> vector nesting, so sizeof OMPTraitInfo is 880 bytes, and then you are passing 
> it by value in in some of the attribute classes. Are you sure you wanted to 
> do that?
I could make them all 0 elements long, reducing the size quite a bit for the 
common case of very few trait sets/selectors/properties. I could also try to 
dynamically allocate them once. WDYT?


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Reid Kleckner via Phabricator via cfe-commits]

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 OMPTraitInfo is 880 bytes, and then you are passing it by 
value in in some of the attribute classes. Are you sure you wanted to do that?


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Florian Hahn via Phabricator via cfe-commits]

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.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

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:
+//
+//   OMPTraitInfo := {list}
+//   OMPTraitSet  := {Kind, list}
+//   OMPTraitSelector := {Kind, Expr, list}
+//   OMPTraitProperty := {Kind}

Rather than describing the internal data structure form, I am hoping for 
comments that show the format of the user-facing pragma arguments. e.g., what 
information is in what position. Basically, something so I can mentally map 
from "OMPTraitsInfoArgument" to "oh, that's right, the arguments are specified 
in this order with these types" (roughly -- a general idea of the argument is 
all I'm looking for).



Comment at: clang/lib/Parse/ParseOpenMP.cpp:1379
   // Parse inner context selectors.
-  SmallVector Data;
-  if (!parseOpenMPContextSelectors(Loc, Data)) {
-// Parse ')'.
-(void)T.consumeClose();
-// Need to check for extra tokens.
-if (Tok.isNot(tok::annot_pragma_openmp_end)) {
-  Diag(Tok, diag::warn_omp_extra_tokens_at_eol)
-  << getOpenMPDirectiveName(OMPD_declare_variant);
-}
-  }
+  OMPTraitInfo *TI = new OMPTraitInfo();
+  parseOMPContextSelectors(Loc, *TI);

jdoerfert wrote:
> jdoerfert wrote:
> > aaron.ballman wrote:
> > > What is responsible for freeing this memory?
> > Will check and fix if needed.
> I made sure all OMPTraitInfo are freed, I think. If an 
> `OMPDeclareVariantAttr` is build its taking owenership and calls delete in 
> the destructor.
That is reasonable enough -- any chance we can easily turn it into a 
`std::unique_ptr<>` and then get rid of the destructor entirely? I don't 
insist, but it would be cleaner, to my thinking.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

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  ";
+OS << "  OS << \" \" << *SA->get" << getUpperName() << "();\n";

aaron.ballman wrote:
> Can we leave the unreachable in there and add actual checking for this case? 
> The unreachable is intended to remind people "do this work when you're 
> mucking about in here" and silently doing something that may or may not be 
> wrong makes this harder to spot.
Overlooked this one, sorry. Will fix it and make it unreachable again.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

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:
> > aaron.ballman wrote:
> > > I didn't see anything that validated that this is known to be a constant 
> > > expression. Did I miss something?
> > > 
> > > Also, is there anything checking that the expression is not value 
> > > dependent?
> > > I didn't see anything that validated that this is known to be a constant 
> > > expression. Did I miss something? 
> > 
> > I'll look into that and make sure we check in the parser.
> > 
> > > Also, is there anything checking that the expression is not value 
> > > dependent?
> > 
> > That should be fine. If it is attached to a template it is instantiated 
> > with the template. Or am I missing something?
> > That should be fine. If it is attached to a template it is instantiated 
> > with the template. Or am I missing something?
> 
> `EvaluateKnownConstInt()` asserts that the expression is not value dependent, 
> so I just wanted to be certain that assertion wouldn't trigger. Sounds like 
> it won't because this should only be called on an instantiation?
I added detection and tests for constant, value-dependent, and non-constant 
expressions in score and user condition. Assuming I didn't miss anything it 
should not assert here.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:1379
   // Parse inner context selectors.
-  SmallVector Data;
-  if (!parseOpenMPContextSelectors(Loc, Data)) {
-// Parse ')'.
-(void)T.consumeClose();
-// Need to check for extra tokens.
-if (Tok.isNot(tok::annot_pragma_openmp_end)) {
-  Diag(Tok, diag::warn_omp_extra_tokens_at_eol)
-  << getOpenMPDirectiveName(OMPD_declare_variant);
-}
-  }
+  OMPTraitInfo *TI = new OMPTraitInfo();
+  parseOMPContextSelectors(Loc, *TI);

jdoerfert wrote:
> aaron.ballman wrote:
> > What is responsible for freeing this memory?
> Will check and fix if needed.
I made sure all OMPTraitInfo are freed, I think. If an `OMPDeclareVariantAttr` 
is build its taking owenership and calls delete in the destructor.



Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:110
   .Case("ParamIdx", "ParamIdx::deserialize(Record.readInt())")
+  .EndsWith("*", "Record.readUserType<" +
+ std::string(type.data(), 0, type.size() - 1) + ">()")

aaron.ballman wrote:
> jdoerfert wrote:
> > aaron.ballman wrote:
> > > The idea here being that if someone adds a `GenericPointerArg`, they'll 
> > > have to add a specialization of `readUserType<>` for that named pointer 
> > > type or else get template instantiation errors because there's no 
> > > definition for the primary template?
> > Correct.
> I like it. :-)
It's gone now ;)


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

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 a bit strange given that it's not a generic pointer, you 
> > specify a type with it. It's unclear whether you are supposed to specify 
> > the pointer type or the base type, which would help a bit. However, it's 
> > unclear to me why we'd want this generic mechanism in the first place. 
> > These classes are intended to help generate code to do checking 
> > automatically, and this design is somewhat at odds with that goal compared 
> > to exposing a custom argument type like we did for `VersionArgument`. Also, 
> > this type is marked optional rather than leaving that up to the caller, and 
> > the default behavior of a generic pointer argument always being default 
> > seems suspect.
> > 
> > I'm not saying "get rid of this", just wondering if you had considered the 
> > more verbose approach and had strong rationale as to why this was a better 
> > way.
> > The name seems a bit strange given that it's not a generic pointer, you 
> > specify a type with it.
> 
> The "template" is generic as it accepts any pointer type. All the code that 
> work with `GenericPointerArgument` don't know what pointer it is, just that 
> it is called "Type". Don't you think that is generic?
> 
> 
> > Also, this type is marked optional rather than leaving that up to the 
> > caller, and the default behavior of a generic pointer argument always being 
> > default seems suspect.
> 
> That's an oversight. I mark it non-optional. I don't get the "always being 
> default" part.
> 
> > However, it's unclear to me why we'd want this generic mechanism in the 
> > first place. 
> > [...]
> > I'm not saying "get rid of this", just wondering if you had considered the 
> > more verbose approach and had strong rationale as to why this was a better 
> > way.
> 
> The "problem" is that we want to keep "complex" information around. The 
> verbose approach is what we are doing right now for a subset of the 
> information. For this subset we already track various variadic expr, variadic 
> unsigned, and variadic string arguments and stitch them together later with 
> complex and careful iteration over all containers at the same time. It's now 
> 5 variadic XXX arguments and it would be >= 7 to support what this approach 
> does.
> 
> This approach subsumes this as we can retain the original structure/nesting 
> in the custom `OMPTraitInfo` object that is part of the `OMPDeclareVariant` 
> instead. [FWIW, in the review for the verbose code we have now, in case you 
> haven't seen that, I asked for a less verbose method to store the information 
> because the iteration over the stuff, once flattend, is so brittle.]
> 
> That said, we can make a specialized argument here instead, e.g., 
> OMPTraitInforArgument, which contains a OMPTraitInfo pointer. I figured this 
> might not be the last time someone wants to keep a complex structure inside 
> an attribute argument and creating new arguments all the time seems a lot of 
> waste (due to all the boilerplate). If you think we should go that route, I 
> will happily try it out.
> 
> (As noted below somewhere, this method avoids a lot of boilerplate by 
> requiring a specialization of one AST reader/writer method.)
> That's an oversight. I mark it non-optional. I don't get the "always being 
> default" part.

Making it non-optional solves my concern. I was just worried that the default 
was surprising.

> That said, we can make a specialized argument here instead, e.g., 
> OMPTraitInforArgument, which contains a OMPTraitInfo pointer. I figured this 
> might not be the last time someone wants to keep a complex structure inside 
> an attribute argument and creating new arguments all the time seems a lot of 
> waste (due to all the boilerplate). If you think we should go that route, I 
> will happily try it out.

As we spoke about on IRC, I would appreciate going with this approach (and I 
think it can even work nicely with the template specialization work done for 
the AST reader and writer, most likely). However, if that turns out to be a lot 
of effort for you, I can probably be okay with the current approach. I just 
dislike the fact that it complicates understanding what arguments the attribute 
actually takes (I no longer understand by looking at Attr.td or in the worst 
case, the tablegen emitter). Having the concrete type in tablegen/emitter is 
more expressive and allows us to generate more stuff in the future.



Comment at: clang/include/clang/Basic/Attr.td:3351
 ExprArgument<"VariantFuncRef">,
-VariadicExprArgument<"Scores">,
-VariadicUnsignedArgument<"CtxSelectorSets">,
-VariadicUnsignedArgument<"CtxSelectors">,
-VariadicStringArgument<"ImplVendors">,
-

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

[Johannes Doerfert via Phabricator via cfe-commits]

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 wrote:
> The name seems a bit strange given that it's not a generic pointer, you 
> specify a type with it. It's unclear whether you are supposed to specify the 
> pointer type or the base type, which would help a bit. However, it's unclear 
> to me why we'd want this generic mechanism in the first place. These classes 
> are intended to help generate code to do checking automatically, and this 
> design is somewhat at odds with that goal compared to exposing a custom 
> argument type like we did for `VersionArgument`. Also, this type is marked 
> optional rather than leaving that up to the caller, and the default behavior 
> of a generic pointer argument always being default seems suspect.
> 
> I'm not saying "get rid of this", just wondering if you had considered the 
> more verbose approach and had strong rationale as to why this was a better 
> way.
> The name seems a bit strange given that it's not a generic pointer, you 
> specify a type with it.

The "template" is generic as it accepts any pointer type. All the code that 
work with `GenericPointerArgument` don't know what pointer it is, just that it 
is called "Type". Don't you think that is generic?


> Also, this type is marked optional rather than leaving that up to the caller, 
> and the default behavior of a generic pointer argument always being default 
> seems suspect.

That's an oversight. I mark it non-optional. I don't get the "always being 
default" part.

> However, it's unclear to me why we'd want this generic mechanism in the first 
> place. 
> [...]
> I'm not saying "get rid of this", just wondering if you had considered the 
> more verbose approach and had strong rationale as to why this was a better 
> way.

The "problem" is that we want to keep "complex" information around. The verbose 
approach is what we are doing right now for a subset of the information. For 
this subset we already track various variadic expr, variadic unsigned, and 
variadic string arguments and stitch them together later with complex and 
careful iteration over all containers at the same time. It's now 5 variadic XXX 
arguments and it would be >= 7 to support what this approach does.

This approach subsumes this as we can retain the original structure/nesting in 
the custom `OMPTraitInfo` object that is part of the `OMPDeclareVariant` 
instead. [FWIW, in the review for the verbose code we have now, in case you 
haven't seen that, I asked for a less verbose method to store the information 
because the iteration over the stuff, once flattend, is so brittle.]

That said, we can make a specialized argument here instead, e.g., 
OMPTraitInforArgument, which contains a OMPTraitInfo pointer. I figured this 
might not be the last time someone wants to keep a complex structure inside an 
attribute argument and creating new arguments all the time seems a lot of waste 
(due to all the boilerplate). If you think we should go that route, I will 
happily try it out.

(As noted below somewhere, this method avoids a lot of boilerplate by requiring 
a specialization of one AST reader/writer method.)



Comment at: clang/include/clang/Basic/Attr.td:3351
 ExprArgument<"VariantFuncRef">,
-VariadicExprArgument<"Scores">,
-VariadicUnsignedArgument<"CtxSelectorSets">,
-VariadicUnsignedArgument<"CtxSelectors">,
-VariadicStringArgument<"ImplVendors">,
-VariadicStringArgument<"DeviceKinds">
+GenericPointerArgument<"TraitInfos", "OMPTraitInfo*">,
   ];

aaron.ballman wrote:
> Just double-checking -- this is not a breaking change to existing code, 
> correct?
> 
> It's not yet clear to me how a "generic pointer argument" relates to an 
> `OMPTraitInfo` object that the end programmer will have no understanding of 
> because it's a compiler implementation detail.
> 
> I used to understand what this attribute accepted as arguments and I no 
> longer have the foggiest idea, which seems somewhat problematic.
> Just double-checking -- this is not a breaking change to existing code, 
> correct?

This actually fixes various errors in the existing code and adds a lot of 
missing functionality, no regressions are known to me.

> It's not yet clear to me how a "generic pointer argument" relates to an 
> OMPTraitInfo object that the end programmer will have no understanding of 
> because it's a compiler implementation detail.

This is not (supposed to be) user facing. This is generated from the `...` in 
`omp declare variant match(...)`.

> I used to understand what this attribute accepted as arguments and I no 
> longer have the foggiest idea, which seems somewhat problematic.

I don't understand? The attribute accepted a custom (=internal) encoding 

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

[Aaron Ballman via Phabricator via cfe-commits]

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 adjusted that to non-blocking, but feel free to leave a comment if I've 
> missed something.


I don't know that we have any formalized procedure here, but setting a code 
owner as the blocking reviewer in phab makes it crystal clear that the existing 
LG(s) are insufficient for some reason and someone needs to do a final 
sign-off. It solves the problem of getting a LG from someone who may not be 
familiar with the code base and then making a problematic commit by accident.




Comment at: clang/include/clang/AST/OpenMPClause.h:6543
 
+/// Helper datastructure representing the traits in a match clause of an
+/// `declare variant` or `metadirective`. The outer level is an ordered

datastructure -> data structure



Comment at: clang/include/clang/AST/OpenMPClause.h:6565
+
+  /// Create a variant match infor object from this trait info object. While 
the
+  /// former is a flat representation the actual main difference is that the

infor -> info



Comment at: clang/include/clang/AST/OpenMPClause.h:6573
+
+  /// Print a humand readable representation into \p OS.
+  void print(llvm::raw_ostream , const PrintingPolicy ) const;

humand -> human



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;

The name seems a bit strange given that it's not a generic pointer, you specify 
a type with it. It's unclear whether you are supposed to specify the pointer 
type or the base type, which would help a bit. However, it's unclear to me why 
we'd want this generic mechanism in the first place. These classes are intended 
to help generate code to do checking automatically, and this design is somewhat 
at odds with that goal compared to exposing a custom argument type like we did 
for `VersionArgument`. Also, this type is marked optional rather than leaving 
that up to the caller, and the default behavior of a generic pointer argument 
always being default seems suspect.

I'm not saying "get rid of this", just wondering if you had considered the more 
verbose approach and had strong rationale as to why this was a better way.



Comment at: clang/include/clang/Basic/Attr.td:3351
 ExprArgument<"VariantFuncRef">,
-VariadicExprArgument<"Scores">,
-VariadicUnsignedArgument<"CtxSelectorSets">,
-VariadicUnsignedArgument<"CtxSelectors">,
-VariadicStringArgument<"ImplVendors">,
-VariadicStringArgument<"DeviceKinds">
+GenericPointerArgument<"TraitInfos", "OMPTraitInfo*">,
   ];

Just double-checking -- this is not a breaking change to existing code, correct?

It's not yet clear to me how a "generic pointer argument" relates to an 
`OMPTraitInfo` object that the end programmer will have no understanding of 
because it's a compiler implementation detail.

I used to understand what this attribute accepted as arguments and I no longer 
have the foggiest idea, which seems somewhat problematic.



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1262
+def warn_omp_declare_variant_string_literal_or_identifier : Warning<
+  "expected identifier or string literal describing a context 
%select{set|selector|property}0, %select{set|selector|property}0 skipped">,
+  InGroup;

80-col limit

Also, I think the comma should be a semicolon so it reads `...describing a 
context set; selector skipped`



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1267
+def warn_omp_declare_variant_expected : Warning<
+  "expected '%0' after the %1, '%0' assumed">,
+  InGroup;

Turn the comma into a semicolon



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1270
+def warn_omp_declare_variant_ctx_not_a_property : Warning<
+  "'%0' is not a valid context property for the context selector '%1' and the 
context set '%2', property ignored">,
+  InGroup;

80-col limit; please run the patch through clang-format (I'll stop commenting 
on these).



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1277
+def warn_omp_declare_variant_ctx_not_a_selector : Warning<
+  "'%0' is not a valid context selector for the context set '%1', selector 
ignored">,
+  InGroup;

Turn the comma into a semicolon



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1280
+def warn_omp_declare_variant_ctx_not_a_set : Warning<
+  "'%0' is not a valid context set in a `declare variant`, set 

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

[Jon Chesterfield via Phabricator via cfe-commits]

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.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kiran Chandramohan via Phabricator via cfe-commits]

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 ASTRecordWriter::writeUserType(OMPTraitInfo *TI) {
+  writeUInt32(TI->Sets.size());

jdoerfert wrote:
> kiranchandramohan wrote:
> > Had to also move this up in the file to avoid an instantiation after use 
> > error.
> I didn't get that but I have it in the header, I'll figure it out.
You should remove the declaration of this specialization from the header file 
first.
Then move this definition of the specialization up in the file.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

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 chance to look).




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

kiranchandramohan wrote:
> jdoerfert wrote:
> > jdoerfert wrote:
> > > kiranchandramohan wrote:
> > > > jdoerfert wrote:
> > > > > kiranchandramohan wrote:
> > > > > > jdoerfert wrote:
> > > > > > > kiranchandramohan wrote:
> > > > > > > > Compiler throws up this error.
> > > > > > > > error: explicit specialization in non-namespace scope ‘class 
> > > > > > > > clang::ASTRecordReader’
> > > > > > > Oh, my compiler was happy. Let me rebase and see what the 
> > > > > > > pre-merge bots say so I might get some insight into the problem.
> > > > > > Were you able to reproduce the error? I was using gcc 9.2 compiler.
> > > > > I have not seen the error yet. I build with clang. Do you happen to 
> > > > > have an idea how to refactor this? I will look into it with a new gcc 
> > > > > asap.
> > > > Moving the specialization to the source file seems to fix the issue.
> > > I will do that then. Did you find the time to look over the rest?
> > > Moving the specialization to the source file seems to fix the issue.
> > 
> > Like this?
> You can remove the following declaration from the header file and just define 
> it in the cpp file. 
>  /// Specialization for OMPTraitInfo*.
>   template <> OMPTraitInfo *readUserType();
Right, will do that, thx!



Comment at: clang/lib/Serialization/ASTWriter.cpp:6575
 
+template <> void ASTRecordWriter::writeUserType(OMPTraitInfo *TI) {
+  writeUInt32(TI->Sets.size());

kiranchandramohan wrote:
> Had to also move this up in the file to avoid an instantiation after use 
> error.
I didn't get that but I have it in the header, I'll figure it out.



Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:417
+#include "llvm/Frontend/OpenMP/OMPKinds.def"
+  S.pop_back();
+  return S;

kiranchandramohan wrote:
> If there is no match? Or is it always guaranteed to have a match?
We always have selector sets (the outermost category). 


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kiran Chandramohan via Phabricator via cfe-commits]

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 after use error.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kiran Chandramohan via Phabricator via cfe-commits]

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 wrote:
> > kiranchandramohan wrote:
> > > jdoerfert wrote:
> > > > kiranchandramohan wrote:
> > > > > jdoerfert wrote:
> > > > > > kiranchandramohan wrote:
> > > > > > > Compiler throws up this error.
> > > > > > > error: explicit specialization in non-namespace scope ‘class 
> > > > > > > clang::ASTRecordReader’
> > > > > > Oh, my compiler was happy. Let me rebase and see what the pre-merge 
> > > > > > bots say so I might get some insight into the problem.
> > > > > Were you able to reproduce the error? I was using gcc 9.2 compiler.
> > > > I have not seen the error yet. I build with clang. Do you happen to 
> > > > have an idea how to refactor this? I will look into it with a new gcc 
> > > > asap.
> > > Moving the specialization to the source file seems to fix the issue.
> > I will do that then. Did you find the time to look over the rest?
> > Moving the specialization to the source file seems to fix the issue.
> 
> Like this?
You can remove the following declaration from the header file and just define 
it in the cpp file. 
 /// Specialization for OMPTraitInfo*.
  template <> OMPTraitInfo *readUserType();



Comment at: llvm/lib/Frontend/OpenMP/OMPContext.cpp:417
+#include "llvm/Frontend/OpenMP/OMPKinds.def"
+  S.pop_back();
+  return S;

If there is no match? Or is it always guaranteed to have a match?


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

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
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

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:
> kiranchandramohan wrote:
> > jdoerfert wrote:
> > > kiranchandramohan wrote:
> > > > jdoerfert wrote:
> > > > > kiranchandramohan wrote:
> > > > > > Compiler throws up this error.
> > > > > > error: explicit specialization in non-namespace scope ‘class 
> > > > > > clang::ASTRecordReader’
> > > > > Oh, my compiler was happy. Let me rebase and see what the pre-merge 
> > > > > bots say so I might get some insight into the problem.
> > > > Were you able to reproduce the error? I was using gcc 9.2 compiler.
> > > I have not seen the error yet. I build with clang. Do you happen to have 
> > > an idea how to refactor this? I will look into it with a new gcc asap.
> > Moving the specialization to the source file seems to fix the issue.
> I will do that then. Did you find the time to look over the rest?
> Moving the specialization to the source file seems to fix the issue.

Like this?


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

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:
> jdoerfert wrote:
> > kiranchandramohan wrote:
> > > jdoerfert wrote:
> > > > kiranchandramohan wrote:
> > > > > Compiler throws up this error.
> > > > > error: explicit specialization in non-namespace scope ‘class 
> > > > > clang::ASTRecordReader’
> > > > Oh, my compiler was happy. Let me rebase and see what the pre-merge 
> > > > bots say so I might get some insight into the problem.
> > > Were you able to reproduce the error? I was using gcc 9.2 compiler.
> > I have not seen the error yet. I build with clang. Do you happen to have an 
> > idea how to refactor this? I will look into it with a new gcc asap.
> Moving the specialization to the source file seems to fix the issue.
I will do that then. Did you find the time to look over the rest?


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.



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

jdoerfert wrote:
> kiranchandramohan wrote:
> > jdoerfert wrote:
> > > kiranchandramohan wrote:
> > > > Compiler throws up this error.
> > > > error: explicit specialization in non-namespace scope ‘class 
> > > > clang::ASTRecordReader’
> > > Oh, my compiler was happy. Let me rebase and see what the pre-merge bots 
> > > say so I might get some insight into the problem.
> > Were you able to reproduce the error? I was using gcc 9.2 compiler.
> I have not seen the error yet. I build with clang. Do you happen to have an 
> idea how to refactor this? I will look into it with a new gcc asap.
Moving the specialization to the source file seems to fix the issue.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

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:
> jdoerfert wrote:
> > kiranchandramohan wrote:
> > > Compiler throws up this error.
> > > error: explicit specialization in non-namespace scope ‘class 
> > > clang::ASTRecordReader’
> > Oh, my compiler was happy. Let me rebase and see what the pre-merge bots 
> > say so I might get some insight into the problem.
> Were you able to reproduce the error? I was using gcc 9.2 compiler.
I have not seen the error yet. I build with clang. Do you happen to have an 
idea how to refactor this? I will look into it with a new gcc asap.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kiran Chandramohan via Phabricator via cfe-commits]

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 this error.
> > error: explicit specialization in non-namespace scope ‘class 
> > clang::ASTRecordReader’
> Oh, my compiler was happy. Let me rebase and see what the pre-merge bots say 
> so I might get some insight into the problem.
Were you able to reproduce the error? I was using gcc 9.2 compiler.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

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 `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 


//Pre-merge checks is in beta. Report issue 
.
 Please join beta  or enable 
it for your project 
.//


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[pre-merge checks [bot] via Phabricator via cfe-commits]

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 
.
 0 of them are added as review comments below (why? 
).

{icon times-circle color=red} clang-format: fail. Please format your changes 
with clang-format by running `git-clang-format HEAD^` or applying this patch 
.

Build artifacts 
: 
diff.json 
,
 clang-tidy.txt 
,
 clang-format.patch 
,
 CMakeCache.txt 
,
 console-log.txt 
,
 test-results.xml 


//Pre-merge checks is in beta. Report issue 
.
 Please join beta  or enable 
it for your project 
.//


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

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 your comments.




Comment at: clang/include/clang/AST/OpenMPClause.h:6429
+/// and an ordered collection of properties.
+struct OpenMPTraitInfo {
+  struct OpenMPTraitProperty {

kiranchandramohan wrote:
> Not clear from this file what should be named starting with OMP or OpenMP. I 
> see more classes/structs strating with OMP.
I will go for OMP everywhere.



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

kiranchandramohan wrote:
> Compiler throws up this error.
> error: explicit specialization in non-namespace scope ‘class 
> clang::ASTRecordReader’
Oh, my compiler was happy. Let me rebase and see what the pre-merge bots say so 
I might get some insight into the problem.



Comment at: clang/include/clang/Serialization/ASTRecordWriter.h:277
+
+  template <> void writeUserType(OpenMPTraitInfo *TI) {
+writeOpenMPTraitInfo(TI);

kiranchandramohan wrote:
> Compiler throws up this error.
> error: explicit specialization in non-namespace scope ‘class 
> clang::ASTRecordWriter’
Same as above I guess.



Comment at: clang/lib/Parse/ParseOpenMP.cpp:853
+llvm::omp::TraitSelector Selector, llvm::StringMap ) {
+  const unsigned Lvl = 2;
+  TIProperty.Kind = TraitProperty::invalid;

kiranchandramohan wrote:
> Would it be better to add the value of this variable also to the name? Like 
> Lvl_TWO or something? Same question for the 5 other const Lvl variables in 
> this patch.
Yes, that would be better. Will fix.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Kiran Chandramohan via Phabricator via cfe-commits]

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 OpenMPTraitProperty {

Not clear from this file what should be named starting with OMP or OpenMP. I 
see more classes/structs strating with OMP.



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

Compiler throws up this error.
error: explicit specialization in non-namespace scope ‘class 
clang::ASTRecordReader’



Comment at: clang/include/clang/Serialization/ASTRecordWriter.h:277
+
+  template <> void writeUserType(OpenMPTraitInfo *TI) {
+writeOpenMPTraitInfo(TI);

Compiler throws up this error.
error: explicit specialization in non-namespace scope ‘class 
clang::ASTRecordWriter’



Comment at: clang/lib/Parse/ParseOpenMP.cpp:853
+llvm::omp::TraitSelector Selector, llvm::StringMap ) {
+  const unsigned Lvl = 2;
+  TIProperty.Kind = TraitProperty::invalid;

Would it be better to add the value of this variable also to the name? Like 
Lvl_TWO or something? Same question for the 5 other const Lvl variables in this 
patch.


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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Johannes Doerfert via Phabricator via cfe-commits]

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
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits