[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/AST/ASTContext.h:1983
+  /// \brief Types and expressions required to build C++2a three-way 
comparisons
+  ///   using operator<=>, including the values return by builtin <=> 
operators.
+  ComparisonCategories CompCategories;

We don't generally indent comments after a `\brief` like this. Also, we enable 
autobrief, so these `\brief`s are redundant.



Comment at: include/clang/AST/ComparisonCategories.h:56
+///   comparison. These values map onto instances of comparison category types
+///   defined in the standard library. i.e. 'std::strong_ordering::less'.
+enum class ComparisonCategoryResult : unsigned char {

You mean "eg", not "ie" here.



Comment at: include/clang/AST/ComparisonCategories.h:84-103
+/// \brief True iff we've successfully evaluated the variable as a constant
+/// expression and extracted its integer value.
+bool hasValidIntValue() const { return HasValue; }
+
+/// \brief Get the constant integer value used by this variable to 
represent
+/// the comparison category result type.
+llvm::APSInt getIntValue() const {

This seems unnecessary; we can get this information from the `VarDecl` instead. 
(You're caching a result here that is already cached there.)



Comment at: lib/AST/ComparisonCategories.cpp:25
+/// category result by evaluating the initializer for the specified VarDecl as
+/// a constant expression and retreiving the value of the classes first
+/// (and only) field.

classes -> class's



Comment at: lib/AST/ComparisonCategories.cpp:43-46
+  Expr::EvalResult Result;
+  if (!Info->VD->hasInit() ||
+  !Info->VD->getInit()->EvaluateAsRValue(Result, Ctx))
+return true;

Use `VD->evaluateValue()` to get the cached value already stored by the 
`VarDecl`.



Comment at: lib/CodeGen/CGExprAgg.cpp:959
+  !ArgTy->isMemberPointerType() && !ArgTy->isAnyComplexType()) {
+return CGF.ErrorUnsupported(E, "aggregate three-way comparisoaoeun");
+  }

Typo. I'm almost tempted to say we should keep this for entertainment value, 
but on balance let's fix it :)



Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2899-2937
   assert(!location.getAs() && "location cannot be a NonLoc.");
+
+  // Are we loading from a region?  This actually results in two loads; one
+  // to fetch the address of the referenced value and one to fetch the
+  // referenced value.
+  if (const auto *TR =
+dyn_cast_or_null(location.getAsRegion())) {

This does not look related to your three-way comparison changes.



Comment at: test/SemaCXX/compare-cxx2a.cpp:40
 
-#if 0
+  (void)(A < 42);
   // (A,b)

Did you intend to add this here? It doesn't look related to the code under test.



Comment at: test/SemaCXX/std-compare-cxx2a.cpp:6
+void compare_not_found_test() {
+  // expected-error@+1 {{cannot deduce return type of 'operator<=>' because 
type partial_ordering was not found; include }}
+  (void)(0.0 <=> 42.123);

This diagnostic says just `partial_ordering` whereas the one below says 
`'std::partial_ordering'`. I prefer the latter more-explicit form, but in any 
case it would seem good to be consistent.


Repository:
  rL LLVM

https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[John McCall via Phabricator via cfe-commits]

rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.

Fine, that works.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: lib/AST/ExprConstant.cpp:8829
+  return EvaluateComparisonBinaryOperator(Info, E, OnSuccess, [&]() {
+return ExprEvaluatorBaseTy::VisitBinaryOperator(E);
+  });

rsmith wrote:
> EricWF wrote:
> > rsmith wrote:
> > > It'd be clearer to call `VisitBinCmp` rather than `VisitBinaryOperator`.
> > @rsmith: OK, so I'm confused about this. Originally I had an 
> > `llvm_unreachable` that the continuation was never reached, but you 
> > suggested it was. I'm not sure how. Could you provide an example?
> > 
> > The precondition of calling `VisitBinCmp` is that we have a call to a 
> > builtin operator. For `<=>`,  where the composite type is either an 
> > arithmetic type, pointer type, or member pointer type (which includes enum 
> > types after conversions),  *All* of these cases should be handled before 
> > reaching the function.
> > 
> > Is there a control flow path I'm missing? 
> What about comparisons of `_Complex` types, vector types, and any other 
> builtin type that gets added after you commit this patch? The right thing to 
> do (at least for now) in all of those cases is to call the base class 
> implementation, which will deal with emitting the "sorry, I don't know how to 
> constant-evaluate this" diagnostic.
> 
> My comment here was simply that when doing so, you should call the base-class 
> version of the *same* function, which you now do, so that concern is 
> addressed.
Ah, I didn't think about how errors were handled. Thank you.




Comment at: lib/CodeGen/CGExprAgg.cpp:964
+RHS = CGF.EmitAnyExpr(E->getRHS()).getAggregatePointer();
+break;
+  case TEK_Complex:

rjmccall wrote:
> EricWF wrote:
> > EricWF wrote:
> > > rjmccall wrote:
> > > > It looks like we don't actually support any aggregate types here, which 
> > > > I think is fine because comparing those types is only sensible for 
> > > > things like calls.  If you do want to pave the way for that, or 
> > > > (probably more usefully) for supporting complex types, you should make 
> > > > EmitCompare take the operands as RValues and just use EmitAnyExpr here 
> > > > without paying any attention to the evaluation kind.
> > > Initially I thought the same thing, but apparently member pointers are 
> > > Aggregates under the Microsoft ABI.
> > > 
> > > I'll give  trafficking in `RValue`s, but all the functions `EmitCompare` 
> > > calls use `Value*`, so it'll take some work.
> > *I'll give trafficking in `RValue`s a shot, but ...*
> Okay, this would be a *lot* cleaner with RValue.  You can break it down in 
> your EmitCmp helper function instead of EmitCompare if you want, but you've 
> basically just inlined EmitAnyExpr here.
OK, I think I've cleaned it up. Let me know what you think.



Comment at: lib/CodeGen/CGExprAgg.cpp:924
+  }();
+  ArgTy->isAnyComplexType();
+  if (ArgTy->hasFloatingRepresentation())

rjmccall wrote:
> Dead code?
Woops. Removed.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/CodeGen/CGExprAgg.cpp:964
+RHS = CGF.EmitAnyExpr(E->getRHS()).getAggregatePointer();
+break;
+  case TEK_Complex:

EricWF wrote:
> EricWF wrote:
> > rjmccall wrote:
> > > It looks like we don't actually support any aggregate types here, which I 
> > > think is fine because comparing those types is only sensible for things 
> > > like calls.  If you do want to pave the way for that, or (probably more 
> > > usefully) for supporting complex types, you should make EmitCompare take 
> > > the operands as RValues and just use EmitAnyExpr here without paying any 
> > > attention to the evaluation kind.
> > Initially I thought the same thing, but apparently member pointers are 
> > Aggregates under the Microsoft ABI.
> > 
> > I'll give  trafficking in `RValue`s, but all the functions `EmitCompare` 
> > calls use `Value*`, so it'll take some work.
> *I'll give trafficking in `RValue`s a shot, but ...*
Okay, this would be a *lot* cleaner with RValue.  You can break it down in your 
EmitCmp helper function instead of EmitCompare if you want, but you've 
basically just inlined EmitAnyExpr here.



Comment at: lib/CodeGen/CGExprAgg.cpp:924
+  }();
+  ArgTy->isAnyComplexType();
+  if (ArgTy->hasFloatingRepresentation())

Dead code?


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/AST/ExprConstant.cpp:8829
+  return EvaluateComparisonBinaryOperator(Info, E, OnSuccess, [&]() {
+return ExprEvaluatorBaseTy::VisitBinaryOperator(E);
+  });

EricWF wrote:
> rsmith wrote:
> > It'd be clearer to call `VisitBinCmp` rather than `VisitBinaryOperator`.
> @rsmith: OK, so I'm confused about this. Originally I had an 
> `llvm_unreachable` that the continuation was never reached, but you suggested 
> it was. I'm not sure how. Could you provide an example?
> 
> The precondition of calling `VisitBinCmp` is that we have a call to a builtin 
> operator. For `<=>`,  where the composite type is either an arithmetic type, 
> pointer type, or member pointer type (which includes enum types after 
> conversions),  *All* of these cases should be handled before reaching the 
> function.
> 
> Is there a control flow path I'm missing? 
What about comparisons of `_Complex` types, vector types, and any other builtin 
type that gets added after you commit this patch? The right thing to do (at 
least for now) in all of those cases is to call the base class implementation, 
which will deal with emitting the "sorry, I don't know how to constant-evaluate 
this" diagnostic.

My comment here was simply that when doing so, you should call the base-class 
version of the *same* function, which you now do, so that concern is addressed.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: lib/AST/ExprConstant.cpp:8829
+  return EvaluateComparisonBinaryOperator(Info, E, OnSuccess, [&]() {
+return ExprEvaluatorBaseTy::VisitBinaryOperator(E);
+  });

rsmith wrote:
> It'd be clearer to call `VisitBinCmp` rather than `VisitBinaryOperator`.
@rsmith: OK, so I'm confused about this. Originally I had an `llvm_unreachable` 
that the continuation was never reached, but you suggested it was. I'm not sure 
how. Could you provide an example?

The precondition of calling `VisitBinCmp` is that we have a call to a builtin 
operator. For `<=>`,  where the composite type is either an arithmetic type, 
pointer type, or member pointer type (which includes enum types after 
conversions),  *All* of these cases should be handled before reaching the 
function.

Is there a control flow path I'm missing? 


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: lib/CodeGen/CGExprAgg.cpp:964
+RHS = CGF.EmitAnyExpr(E->getRHS()).getAggregatePointer();
+break;
+  case TEK_Complex:

EricWF wrote:
> rjmccall wrote:
> > It looks like we don't actually support any aggregate types here, which I 
> > think is fine because comparing those types is only sensible for things 
> > like calls.  If you do want to pave the way for that, or (probably more 
> > usefully) for supporting complex types, you should make EmitCompare take 
> > the operands as RValues and just use EmitAnyExpr here without paying any 
> > attention to the evaluation kind.
> Initially I thought the same thing, but apparently member pointers are 
> Aggregates under the Microsoft ABI.
> 
> I'll give  trafficking in `RValue`s, but all the functions `EmitCompare` 
> calls use `Value*`, so it'll take some work.
*I'll give trafficking in `RValue`s a shot, but ...*


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 8 inline comments as done.
EricWF added inline comments.



Comment at: include/clang/AST/ComparisonCategories.h:71
+  /// standard library. The key is a value of ComparisonCategoryResult.
+  mutable llvm::DenseMap Objects;
+

rjmccall wrote:
> EricWF wrote:
> > rjmccall wrote:
> > > We expect this map to have at least two of the seven result types, and 
> > > probably three or four, right?  It should probably just be an array; 
> > > it'll both be faster and use less memory.
> > > 
> > > (The similar map in `ComparisonCategories` is different because we expect 
> > > it to be empty in most translation units.)
> > Ack.
> > 
> > Do you want `std::array` or something slightly more conservative like 
> > `llvm::SmallVector`?
> std::array is definitely better here.
I went with `SmallVector`, because `ComparisonCategoryinfo` currently has the 
invariant that it always contains a valid `VarDecl`. When I implemented this 
using `std::array` removing that invariant made a lot of code more messy.





Comment at: lib/CodeGen/CGExprAgg.cpp:971
+  auto EmitCmpRes = [&](const VarDecl *VD) {
+return CGF.CGM.GetAddrOfGlobalVar(VD);
+  };

rjmccall wrote:
> EricWF wrote:
> > rjmccall wrote:
> > > EricWF wrote:
> > > > rsmith wrote:
> > > > > Perhaps directly emit the constant value here rather than the address 
> > > > > of the global? I think we should consider what IR we want to see 
> > > > > coming out of Clang, and I don't think that IR should contain loads 
> > > > > from globals to get the small constant integer that is the value of 
> > > > > the conversion result.
> > > > > 
> > > > > I think it would be reasonable for us to say that we require the 
> > > > > standard library types to contain exactly one non-static data member 
> > > > > of integral type, and for us to form a select between the relevant 
> > > > > integer values here. We really have no need to support all possible 
> > > > > implementations of these types, and we can revisit this if some other 
> > > > > standard library implementation ships types that don't follow that 
> > > > > pattern. (If we find such a standard library, we could emit multiple 
> > > > > selects, or a first-class aggregate select, or whatever generates the 
> > > > > best code at -O0.)
> > > > I agree emitting the value would be better, and that most STL 
> > > > implementations should implement the types using only one non-static 
> > > > member.
> > > > However, note that the specification for `partial_ordering` is 
> > > > described in terms of two non-static data members, so it seems possible 
> > > > an STL implementation might implement in that way.
> > > > 
> > > > Would it be appropriate to do this as a smaller follow up patch?
> > > While it would be nice if we could special-case the case of a class with 
> > > a single integral field all the way through the various uses of it, IRGen 
> > > is not at all set up to try to take advantage of that.  You will need to 
> > > store your integral result into the dest slot here anyway.  That makes me 
> > > suspect that there's just no value in trying to produce a scalar select 
> > > before doing that store instead of branching to pick one of two stores.
> > > 
> > > Also, I know there's this whole clever argument for why we can get away 
> > > with lazily finding this comparison-result type and its static members in 
> > > translation units that are just deserializing a spaceship operator.  Just 
> > > to make me feel better, though, please actually check here dynamically 
> > > that the assumptions we're relying on actually hold and that we've found 
> > > an appropriate variable for the comparison result and it does have an 
> > > initializer.  It is fine to generate an atrocious diagnostic if we see a 
> > > failure, but let's please not crash just because something weird and 
> > > unexpected happened with module import.
> > > While it would be nice if we could special-case the case of a class with 
> > > a single integral field all the way through the various uses of it, IRGen 
> > > is not at all set up to try to take advantage of that. You will need to 
> > > store your integral result into the dest slot here anyway. That makes me 
> > > suspect that there's just no value in trying to produce a scalar select 
> > > before doing that store instead of branching to pick one of two stores.
> > 
> > I went ahead and did it anyway, though I suspect you might be right. I'll 
> > need to look into it further. (In particular if we avoid ODR uses and hence 
> > can avoid emitting the inline variable definitions).
> > 
> > > Just to make me feel better, though, please actually check here 
> > > dynamically that the assumptions we're relying on actually hold and that 
> > > we've found an appropriate variable for the comparison result and it does 
> > > have an initializer. 
> > 
> > Ack. I've already added checks in 

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/CodeGen/CGExprAgg.cpp:964
+RHS = CGF.EmitAnyExpr(E->getRHS()).getAggregatePointer();
+break;
+  case TEK_Complex:

It looks like we don't actually support any aggregate types here, which I think 
is fine because comparing those types is only sensible for things like calls.  
If you do want to pave the way for that, or (probably more usefully) for 
supporting complex types, you should make EmitCompare take the operands as 
RValues and just use EmitAnyExpr here without paying any attention to the 
evaluation kind.



Comment at: lib/CodeGen/CGExprAgg.cpp:1000
+  assert(CmpInfo.Record->isTriviallyCopyable() &&
+ "cannot copy non-trivially copyable aggregate");
+

This assertion should really be further up in this function, because you're 
already relying on this quite heavily by this point.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/ComparisonCategories.h:71
+  /// standard library. The key is a value of ComparisonCategoryResult.
+  mutable llvm::DenseMap Objects;
+

EricWF wrote:
> rjmccall wrote:
> > We expect this map to have at least two of the seven result types, and 
> > probably three or four, right?  It should probably just be an array; it'll 
> > both be faster and use less memory.
> > 
> > (The similar map in `ComparisonCategories` is different because we expect 
> > it to be empty in most translation units.)
> Ack.
> 
> Do you want `std::array` or something slightly more conservative like 
> `llvm::SmallVector`?
std::array is definitely better here.



Comment at: lib/AST/ComparisonCategories.cpp:85
+  return nullptr;
+}
+

EricWF wrote:
> rjmccall wrote:
> > This method is returning a pointer to an entry of a DenseMap.  The 
> > resulting pointer is then treated as a stable key in a set on Sema.  That 
> > pointer will be dangling if the DenseMap needs to grow beyond its original 
> > allocation.
> > 
> > I would suggest perhaps storing a fixed-size array of pointers to 
> > ComparisonCategoryInfos that you allocate on-demand.
> Woops! Thanks for the correction. I'm so used to STL node-based maps I 
> assumed the keys were stable.
> 
> I'll use a bitset, and index into it using the `ComparisonCategoryType` 
> enumerators as indexes.
Sounds good.



Comment at: lib/CodeGen/CGExprAgg.cpp:971
+  auto EmitCmpRes = [&](const VarDecl *VD) {
+return CGF.CGM.GetAddrOfGlobalVar(VD);
+  };

EricWF wrote:
> rjmccall wrote:
> > EricWF wrote:
> > > rsmith wrote:
> > > > Perhaps directly emit the constant value here rather than the address 
> > > > of the global? I think we should consider what IR we want to see coming 
> > > > out of Clang, and I don't think that IR should contain loads from 
> > > > globals to get the small constant integer that is the value of the 
> > > > conversion result.
> > > > 
> > > > I think it would be reasonable for us to say that we require the 
> > > > standard library types to contain exactly one non-static data member of 
> > > > integral type, and for us to form a select between the relevant integer 
> > > > values here. We really have no need to support all possible 
> > > > implementations of these types, and we can revisit this if some other 
> > > > standard library implementation ships types that don't follow that 
> > > > pattern. (If we find such a standard library, we could emit multiple 
> > > > selects, or a first-class aggregate select, or whatever generates the 
> > > > best code at -O0.)
> > > I agree emitting the value would be better, and that most STL 
> > > implementations should implement the types using only one non-static 
> > > member.
> > > However, note that the specification for `partial_ordering` is described 
> > > in terms of two non-static data members, so it seems possible an STL 
> > > implementation might implement in that way.
> > > 
> > > Would it be appropriate to do this as a smaller follow up patch?
> > While it would be nice if we could special-case the case of a class with a 
> > single integral field all the way through the various uses of it, IRGen is 
> > not at all set up to try to take advantage of that.  You will need to store 
> > your integral result into the dest slot here anyway.  That makes me suspect 
> > that there's just no value in trying to produce a scalar select before 
> > doing that store instead of branching to pick one of two stores.
> > 
> > Also, I know there's this whole clever argument for why we can get away 
> > with lazily finding this comparison-result type and its static members in 
> > translation units that are just deserializing a spaceship operator.  Just 
> > to make me feel better, though, please actually check here dynamically that 
> > the assumptions we're relying on actually hold and that we've found an 
> > appropriate variable for the comparison result and it does have an 
> > initializer.  It is fine to generate an atrocious diagnostic if we see a 
> > failure, but let's please not crash just because something weird and 
> > unexpected happened with module import.
> > While it would be nice if we could special-case the case of a class with a 
> > single integral field all the way through the various uses of it, IRGen is 
> > not at all set up to try to take advantage of that. You will need to store 
> > your integral result into the dest slot here anyway. That makes me suspect 
> > that there's just no value in trying to produce a scalar select before 
> > doing that store instead of branching to pick one of two stores.
> 
> I went ahead and did it anyway, though I suspect you might be right. I'll 
> need to look into it further. (In particular if we avoid ODR uses and hence 
> can avoid emitting the inline variable definitions).
> 
> > 

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 12 inline comments as done.
EricWF added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:9825
+LHS = S.ImpCastExprToType(LHS.get(), IntType, CK_IntegralCast);
+RHS = S.ImpCastExprToType(RHS.get(), IntType, CK_IntegralCast);
+LHSType = RHSType = IntType;

EricWF wrote:
> rjmccall wrote:
> > I believe this will assert if the underlying type of the enum is `bool`.
> Ack. It does indeed.
> 
> What's the correct way to promote the boolean to an integer?
I seem to have solved the problem for enums with an underlying type of bool by 
first performing LValueToRValue conversions, followed by a conversion to `int` 
instead of `bool`.

Does that sound reasonable?



Comment at: lib/Sema/SemaExpr.cpp:9955
+  // and direction polls
+  return buildResultTy(ComparisonCategoryType::StrongEquality);
+

rjmccall wrote:
> Should we generate a tautological warning about comparisons on `nullptr_t` 
> that aren't the result of some kind of macro/template expansion?
Probably? But we don't currently do it for equality operators, so perhaps this 
could be done in a follow up patch which adds the diagnostic for both equality 
and three-way comparisons?


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 18 inline comments as done.
EricWF added inline comments.



Comment at: include/clang/AST/ASTContext.h:1986
+  /// This object needs to be initialized by Sema the first time it checks
+  /// a three-way comparison.
+  ComparisonCategories CompCategories;

rjmccall wrote:
> Is this comment accurate?  Because if your test case with a deserialized 
> comparison operator is supposed to work, we certainly don't get that.
Woops. No, it is not accurate. It's a remnant of an earlier attempt. I'll 
remove it.



Comment at: include/clang/AST/ComparisonCategories.h:71
+  /// standard library. The key is a value of ComparisonCategoryResult.
+  mutable llvm::DenseMap Objects;
+

rjmccall wrote:
> We expect this map to have at least two of the seven result types, and 
> probably three or four, right?  It should probably just be an array; it'll 
> both be faster and use less memory.
> 
> (The similar map in `ComparisonCategories` is different because we expect it 
> to be empty in most translation units.)
Ack.

Do you want `std::array` or something slightly more conservative like 
`llvm::SmallVector`?



Comment at: lib/AST/ComparisonCategories.cpp:85
+  return nullptr;
+}
+

rjmccall wrote:
> This method is returning a pointer to an entry of a DenseMap.  The resulting 
> pointer is then treated as a stable key in a set on Sema.  That pointer will 
> be dangling if the DenseMap needs to grow beyond its original allocation.
> 
> I would suggest perhaps storing a fixed-size array of pointers to 
> ComparisonCategoryInfos that you allocate on-demand.
Woops! Thanks for the correction. I'm so used to STL node-based maps I assumed 
the keys were stable.

I'll use a bitset, and index into it using the `ComparisonCategoryType` 
enumerators as indexes.



Comment at: lib/CodeGen/CGExprAgg.cpp:971
+  auto EmitCmpRes = [&](const VarDecl *VD) {
+return CGF.CGM.GetAddrOfGlobalVar(VD);
+  };

rjmccall wrote:
> EricWF wrote:
> > rsmith wrote:
> > > Perhaps directly emit the constant value here rather than the address of 
> > > the global? I think we should consider what IR we want to see coming out 
> > > of Clang, and I don't think that IR should contain loads from globals to 
> > > get the small constant integer that is the value of the conversion result.
> > > 
> > > I think it would be reasonable for us to say that we require the standard 
> > > library types to contain exactly one non-static data member of integral 
> > > type, and for us to form a select between the relevant integer values 
> > > here. We really have no need to support all possible implementations of 
> > > these types, and we can revisit this if some other standard library 
> > > implementation ships types that don't follow that pattern. (If we find 
> > > such a standard library, we could emit multiple selects, or a first-class 
> > > aggregate select, or whatever generates the best code at -O0.)
> > I agree emitting the value would be better, and that most STL 
> > implementations should implement the types using only one non-static member.
> > However, note that the specification for `partial_ordering` is described in 
> > terms of two non-static data members, so it seems possible an STL 
> > implementation might implement in that way.
> > 
> > Would it be appropriate to do this as a smaller follow up patch?
> While it would be nice if we could special-case the case of a class with a 
> single integral field all the way through the various uses of it, IRGen is 
> not at all set up to try to take advantage of that.  You will need to store 
> your integral result into the dest slot here anyway.  That makes me suspect 
> that there's just no value in trying to produce a scalar select before doing 
> that store instead of branching to pick one of two stores.
> 
> Also, I know there's this whole clever argument for why we can get away with 
> lazily finding this comparison-result type and its static members in 
> translation units that are just deserializing a spaceship operator.  Just to 
> make me feel better, though, please actually check here dynamically that the 
> assumptions we're relying on actually hold and that we've found an 
> appropriate variable for the comparison result and it does have an 
> initializer.  It is fine to generate an atrocious diagnostic if we see a 
> failure, but let's please not crash just because something weird and 
> unexpected happened with module import.
> While it would be nice if we could special-case the case of a class with a 
> single integral field all the way through the various uses of it, IRGen is 
> not at all set up to try to take advantage of that. You will need to store 
> your integral result into the dest slot here anyway. That makes me suspect 
> that there's just no value in trying to produce a scalar select before doing 
> that store 

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: include/clang/AST/ASTContext.h:1986
+  /// This object needs to be initialized by Sema the first time it checks
+  /// a three-way comparison.
+  ComparisonCategories CompCategories;

Is this comment accurate?  Because if your test case with a deserialized 
comparison operator is supposed to work, we certainly don't get that.



Comment at: include/clang/AST/ComparisonCategories.h:71
+  /// standard library. The key is a value of ComparisonCategoryResult.
+  mutable llvm::DenseMap Objects;
+

We expect this map to have at least two of the seven result types, and probably 
three or four, right?  It should probably just be an array; it'll both be 
faster and use less memory.

(The similar map in `ComparisonCategories` is different because we expect it to 
be empty in most translation units.)



Comment at: include/clang/AST/ComparisonCategories.h:206
+  const ASTContext 
+  mutable llvm::DenseMap Data;
+  mutable NamespaceDecl *StdNS = nullptr;

Please add a comment explaining what the keys are.



Comment at: lib/AST/ComparisonCategories.cpp:85
+  return nullptr;
+}
+

This method is returning a pointer to an entry of a DenseMap.  The resulting 
pointer is then treated as a stable key in a set on Sema.  That pointer will be 
dangling if the DenseMap needs to grow beyond its original allocation.

I would suggest perhaps storing a fixed-size array of pointers to 
ComparisonCategoryInfos that you allocate on-demand.



Comment at: lib/CodeGen/CGExprAgg.cpp:971
+  auto EmitCmpRes = [&](const VarDecl *VD) {
+return CGF.CGM.GetAddrOfGlobalVar(VD);
+  };

EricWF wrote:
> rsmith wrote:
> > Perhaps directly emit the constant value here rather than the address of 
> > the global? I think we should consider what IR we want to see coming out of 
> > Clang, and I don't think that IR should contain loads from globals to get 
> > the small constant integer that is the value of the conversion result.
> > 
> > I think it would be reasonable for us to say that we require the standard 
> > library types to contain exactly one non-static data member of integral 
> > type, and for us to form a select between the relevant integer values here. 
> > We really have no need to support all possible implementations of these 
> > types, and we can revisit this if some other standard library 
> > implementation ships types that don't follow that pattern. (If we find such 
> > a standard library, we could emit multiple selects, or a first-class 
> > aggregate select, or whatever generates the best code at -O0.)
> I agree emitting the value would be better, and that most STL implementations 
> should implement the types using only one non-static member.
> However, note that the specification for `partial_ordering` is described in 
> terms of two non-static data members, so it seems possible an STL 
> implementation might implement in that way.
> 
> Would it be appropriate to do this as a smaller follow up patch?
While it would be nice if we could special-case the case of a class with a 
single integral field all the way through the various uses of it, IRGen is not 
at all set up to try to take advantage of that.  You will need to store your 
integral result into the dest slot here anyway.  That makes me suspect that 
there's just no value in trying to produce a scalar select before doing that 
store instead of branching to pick one of two stores.

Also, I know there's this whole clever argument for why we can get away with 
lazily finding this comparison-result type and its static members in 
translation units that are just deserializing a spaceship operator.  Just to 
make me feel better, though, please actually check here dynamically that the 
assumptions we're relying on actually hold and that we've found an appropriate 
variable for the comparison result and it does have an initializer.  It is fine 
to generate an atrocious diagnostic if we see a failure, but let's please not 
crash just because something weird and unexpected happened with module import.



Comment at: lib/Sema/SemaDeclCXX.cpp:8929
+return QualType();
+  }
+

Alright, works for me.



Comment at: lib/Sema/SemaDeclCXX.cpp:8956
+  // the cache and return the newly cached value.
+  FullyCheckedComparisonCategories.insert(Info);
+  return Info->getType();

I think you should probably do this insertion above (perhaps instead of the 
original `count` check) so that you don't dump 100 diagnostics on the user if 
they've got a malformed stdlib.



Comment at: lib/Sema/SemaExpr.cpp:9816
+RHS = S.ImpCastExprToType(RHS.get(), Type, CK_BitCast);
+  } else {
+// C++2a [expr.spaceship]p4

rsmith wrote:
> EricWF wrote:
> > 

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked an inline comment as done.
EricWF added inline comments.



Comment at: lib/CodeGen/CGExprAgg.cpp:971
+  auto EmitCmpRes = [&](const VarDecl *VD) {
+return CGF.CGM.GetAddrOfGlobalVar(VD);
+  };

rsmith wrote:
> Perhaps directly emit the constant value here rather than the address of the 
> global? I think we should consider what IR we want to see coming out of 
> Clang, and I don't think that IR should contain loads from globals to get the 
> small constant integer that is the value of the conversion result.
> 
> I think it would be reasonable for us to say that we require the standard 
> library types to contain exactly one non-static data member of integral type, 
> and for us to form a select between the relevant integer values here. We 
> really have no need to support all possible implementations of these types, 
> and we can revisit this if some other standard library implementation ships 
> types that don't follow that pattern. (If we find such a standard library, we 
> could emit multiple selects, or a first-class aggregate select, or whatever 
> generates the best code at -O0.)
I agree emitting the value would be better, and that most STL implementations 
should implement the types using only one non-static member.
However, note that the specification for `partial_ordering` is described in 
terms of two non-static data members, so it seems possible an STL 
implementation might implement in that way.

Would it be appropriate to do this as a smaller follow up patch?


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/AST/ExprConstant.cpp:8829
+  return EvaluateComparisonBinaryOperator(Info, E, OnSuccess, [&]() {
+return ExprEvaluatorBaseTy::VisitBinaryOperator(E);
+  });

It'd be clearer to call `VisitBinCmp` rather than `VisitBinaryOperator`.



Comment at: lib/CodeGen/CGExprAgg.cpp:971
+  auto EmitCmpRes = [&](const VarDecl *VD) {
+return CGF.CGM.GetAddrOfGlobalVar(VD);
+  };

Perhaps directly emit the constant value here rather than the address of the 
global? I think we should consider what IR we want to see coming out of Clang, 
and I don't think that IR should contain loads from globals to get the small 
constant integer that is the value of the conversion result.

I think it would be reasonable for us to say that we require the standard 
library types to contain exactly one non-static data member of integral type, 
and for us to form a select between the relevant integer values here. We really 
have no need to support all possible implementations of these types, and we can 
revisit this if some other standard library implementation ships types that 
don't follow that pattern. (If we find such a standard library, we could emit 
multiple selects, or a first-class aggregate select, or whatever generates the 
best code at -O0.)


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D45476#1088189, @rsmith wrote:

> This is significantly more complexity than we need. We're talking about 
> constexpr variables here, so we just need a `VarDecl* -- you can then ask 
> that declaration for its evaluated value and emit that directly.


If these variables are required to satisfy the requirements for that, then I 
agree that that would be the simplest solution.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D45476#1088180, @EricWF wrote:

> OK, As I see it, we have two choices:
>
> (1) Stash the expressions in Sema and import them like
>
> In https://reviews.llvm.org/D45476#1088047, @rjmccall wrote:
>
> > I'm sorry, but this is just not true.  The formation rules for a copy 
> > constructor are laid
> >  out in [class.copy]p2, and they explicitly allow default arguments.
>
>
> Don't apologize because I'm wrong :-P. Thanks for the correction.


It's been a thorn in my side quite a bit, too. :)

>>> Second, in the synopsis for the STL types, no constructors are declared. 
>>> Although I don't
>>>  think the standard spells it out anywhere, I believe this forbids the 
>>> types from having user
>>>  defined constructors (though perhaps not user-declared explicitly 
>>> defaulted constructors.
>>>  For example adding a user defined destructor would be non-conforming since 
>>> it makes
>>>  the types non-literal).
>> 
>> That would certainly be helpful, but I don't think it's true; in general the 
>> standard describes
>>  what things are guaranteed to work with library types, but it doesn't 
>> generally constrain
>>  the existence of other operations.
> 
> I think this is somewhat of a moot point, but I think the constraint is in 
> `[member.functions]p2`:
> 
>> For a non-virtual member function described in the C++ standard library, an 
>> implementation may declare
>>  a different set of member function signatures, provided that any call to 
>> the member function that would
>>  select an overload from the set of declarations described in this document 
>> behaves as if that overload were selected.
> 
> My argument is that because the class synopsis for the comparison category 
> types doesn't describe or declare
>  the copy constructor, so the implementation *isn't*  free to declare it 
> differently.

The type has to allow itself to be constructed with an l-value (whether const 
or not)
of its own type in order to be CopyConstructible.  However, that's just a 
statement
about the *signatures* of the type's constructors; it doesn't say anything 
about whether
those constructors are user-defined, nor does it limit what other constructors 
might
be provided as long as they don't somehow prevent copy-construction from 
succeeding.
(And in somewhat obscure cases, constructing a T with an l-value of type T won't
even select a copy constructor, and that's legal under the standard, too.)  All 
that matters,
absent requirements about T being an aggregate or trivially-copyable type, is 
that the
construction is well-formed and that it has the effect of copying the value.

Anyway, I agree that this is moot.

John.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

This is significantly more complexity than we need. We're talking about 
constexpr variables here, so we just need a `VarDecl* -- you can then ask that 
declaration for its evaluated value and emit that directly.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

OK, As I see it, we have two choices:

(1) Stash the expressions in Sema and import them like

In https://reviews.llvm.org/D45476#1088047, @rjmccall wrote:

>




> Ah.  If you want to be able to find this thing without a Sema around in order 
> to
>  handle deserialized expressions by just caching things in the ASTContext, 
> yes,
>  I agree that it would be difficult to build a `CXXConstructExpr` correctly.  
> I don't
>  fully understand the goal of avoiding serialization here, though.

Perhaps I don't fully understand the goal of avoiding serialization either.

>>> STLs *frequently* make use of default arguments on copy constructors (for
>>>  allocators). I agree that it’s unlikely that that would happen here, but
>>>  that’s precisely because it’s unlikely that this type would ever be
>>>  non-trivial.
>> 
>> A couple of points. First, when I say copy constructor, I mean the special 
>> member, which
>>  cannot have default arguments,
> 
> I'm sorry, but this is just not true.  The formation rules for a copy 
> constructor are laid
>  out in [class.copy]p2, and they explicitly allow default arguments.

Don't apologize because I'm wrong :-P. Thanks for the correction.

>> Second, in the synopsis for the STL types, no constructors are declared. 
>> Although I don't
>>  think the standard spells it out anywhere, I believe this forbids the types 
>> from having user
>>  defined constructors (though perhaps not user-declared explicitly defaulted 
>> constructors.
>>  For example adding a user defined destructor would be non-conforming since 
>> it makes
>>  the types non-literal).
> 
> That would certainly be helpful, but I don't think it's true; in general the 
> standard describes
>  what things are guaranteed to work with library types, but it doesn't 
> generally constrain
>  the existence of other operations.

I think this is somewhat of a moot point, but I think the constraint is in 
`[member.functions]p2`:

> For a non-virtual member function described in the C++ standard library, an 
> implementation may declare
>  a different set of member function signatures, provided that any call to the 
> member function that would
>  select an overload from the set of declarations described in this document 
> behaves as if that overload were selected.

My argument is that because the class synopsis for the comparison category 
types doesn't describe or declare
the copy constructor, so the implementation *isn't*  free to declare it 
differently.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In https://reviews.llvm.org/D45476#1087487, @EricWF wrote:

> In https://reviews.llvm.org/D45476#1087446, @cfe-commits wrote:
>
> > I think you and Richard agreed that you weren’t going to synthesize a whole
> >  expression tree at every use of the operator, and I agree with that
> >  decision. That’s very different from what I’m asking you to do, which is to
> >  synthesize in isolation a call to the copy-constructor.
>
>
> Perhaps. My apologies. I'm still quite new to the Clang internals. I 
> appreciate your patience.
>
> > There are several places in the compiler that require these implicit copies 
> > which aren’t just
> >  normal expressions; this is the common pattern for handling them. The
> >  synthesized expression can be emitted multiple times, and it can be freely
> >  re-synthesized in different translation units instead of being serialized.
>
> I'm not sure this is always the case. For example:
>
>   // foo.h -- compiled as module.
>   #include 
>   struct Foo { int x; };
>   inline auto operator<=>(Foo const& LHS, Foo const& RHS) {
> // CXXConstructExpr's would be built when initially building the 
> expression
> // below. But the caches in ASTContext would not be serialized.
> return LHS.x <=> RHS.x;
>   }
>   // foo.cpp
>   #include  // imported via module.
>   auto bar(Foo LHS, Foo RHS) {
> // The expression below calls a user defined comparison operator,
> // so Sema doesn't process any of the types in , but it
> // does generate code for the inline function, which requires ;
> // but it's too late to synthesize a CXXConstructExpr. 
> return LHS <=> RHS;
>   }
>
>
>
>
> > You’re already finding and caching a constructor; storing a
> >  CXXConstructExpr is basically thr same thing, but in a nicer form that
> >  handles more cases and doesn’t require as much redundant code in IRGen.
>
> I'm not actually caching the copy constructor. And I disagree that storing a
>  `CXXConstructExpr` is essentially the same thing. I can lookup the 
> `CXXConstructorDecl` without `Sema`,
>  but I can't build a `CXXConstructExpr` without it.


Ah.  If you want to be able to find this thing without a Sema around in order to
handle deserialized expressions by just caching things in the ASTContext, yes,
I agree that it would be difficult to build a `CXXConstructExpr` correctly.  I 
don't
fully understand the goal of avoiding serialization here, though.

>> STLs *frequently* make use of default arguments on copy constructors (for
>>  allocators). I agree that it’s unlikely that that would happen here, but
>>  that’s precisely because it’s unlikely that this type would ever be
>>  non-trivial.
> 
> A couple of points. First, when I say copy constructor, I mean the special 
> member, which
>  cannot have default arguments,

I'm sorry, but this is just not true.  The formation rules for a copy 
constructor are laid
out in [class.copy]p2, and they explicitly allow default arguments.

> Also note that it's always the case that at least one copy constructor 
> participates
>  in overload resolution.

But it's not true that that copy constructor has to be selected by overload 
resolution.

> Second, in the synopsis for the STL types, no constructors are declared. 
> Although I don't
>  think the standard spells it out anywhere, I believe this forbids the types 
> from having user
>  defined constructors (though perhaps not user-declared explicitly defaulted 
> constructors.
>  For example adding a user defined destructor would be non-conforming since 
> it makes
>  the types non-literal).

That would certainly be helpful, but I don't think it's true; in general the 
standard describes
what things are guaranteed to work with library types, but it doesn't generally 
constrain
the existence of other operations.

> Third, the STL spec implicitly requires the comparison category types be 
> `CopyConstructible`
>  (The comparison operators are pass by value, and the valid values are 
> declared as const).

Yes, of course.

> Barring STL maintainers that are out of their mind, I posit that the copy 
> constructor
>  `T(T const&)` will always be available.  However, the STL types are free to 
> add base
>  classes and fields arbitrarily. I could imagine some weird reasons why STL's 
> might
>  want to have non-trivial members or bases.

I think it is reasonable to assume that these types will always be 
copy-constructible
from a const l-value, but as mentioned above, that doesn't mean the 
copy-constructor
has to be declared as `T(T const &)`.

>> Mostly, though, I don’t understand the point of imposing a partial set of
>>  non-conformant restrictions on the type. It’s easy to support an arbitrary
>>  copy constructor by synthesizing a CXXConstructExpr, and this will
>>  magically take care of any constexpr issues, as well as removing the need
>>  for open-coding a constructor call.
> 
> Fully checking the validity of copy construction would be preferable. I'll
>  

Re: [PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via cfe-commits]

Woops. Submitted that last comment too early. Editing it on Phab.

On Fri, May 4, 2018 at 2:31 AM, Eric Fiselier via Phabricator <
revi...@reviews.llvm.org> wrote:

> EricWF added a comment.
>
> In https://reviews.llvm.org/D45476#1087446, @cfe-commits wrote:
>
> > I think you and Richard agreed that you weren’t going to synthesize a
> whole
> >  expression tree at every use of the operator, and I agree with that
> >  decision. That’s very different from what I’m asking you to do, which
> is to
> >  synthesize in isolation a call to the copy-constructor.
>
>
> Perhaps. My apologies. I'm still quite new to the Clang internals. I
> appreciate your patience.
>
> > There are several places in the compiler that require these implicit
> copies which aren’t just
> >  normal expressions; this is the common pattern for handling them. The
> >  synthesized expression can be emitted multiple times, and it can be
> freely
> >  re-synthesized in different translation units instead of being
> serialized.
>
> I'm not sure this is always the case. For example:
>
>   // foo.h
>   #include 
>
>   struct Foo {
> int x;
>   };
>   inline auto operator<=>(Foo const& LHS, Foo const& RHS) {
>
>   }
>   // foo.cpp
>   #include  // imported via module.
>   auto bar(Foo LHS, Foo RHS) {
> return
>   }
>
>
>
> > You’re already finding and caching a constructor; storing a
> >  CXXConstructExpr is basically thr same thing, but in a nicer form that
> >  handles more cases and doesn’t require as much redundant code in IRGen.
>
> I'm not actually caching the copy constructor. And I disagree that storing
> a
> `CXXConstructExpr` is essentially the same thing. I can lookup the
> `CXXConstructorDecl` without `Sema`,
> but I can't build a `CXXConstructExpr` without it.
>
> > STLs *frequently* make use of default arguments on copy constructors (for
> >  allocators). I agree that it’s unlikely that that would happen here, but
> >  that’s precisely because it’s unlikely that this type would ever be
> >  non-trivial.
> >
> > Mostly, though, I don’t understand the point of imposing a partial set of
> >  non-conformant restrictions on the type. It’s easy to support an
> arbitrary
> >  copy constructor by synthesizing a CXXConstructExpr, and this will
> >  magically take care of any constexpr issues, as well as removing the
> need
> >  for open-coding a constructor call.
> >
> > The constexpr issues are that certain references to constexpr variables
> of
> >  literal type (as these types are likely to be) are required to not
> ODR-use
> >  the variable but instead just directly produce the initializer as the
> >  expression result.  That’s especially important here because (1)
> existing
> >  STL binaries will not define these variables, and we shouldn’t create
> >  artificial deployment problems for this feature, and (2) we’d really
> rather
> >  not emit these expressions as loads from externally-defined variables
> that
> >  the optimizer won’t be able to optimize.
> >
> > John.
>
>
> https://reviews.llvm.org/D45476
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added a comment.

In https://reviews.llvm.org/D45476#1087446, @cfe-commits wrote:

> I think you and Richard agreed that you weren’t going to synthesize a whole
>  expression tree at every use of the operator, and I agree with that
>  decision. That’s very different from what I’m asking you to do, which is to
>  synthesize in isolation a call to the copy-constructor.


Perhaps. My apologies. I'm still quite new to the Clang internals. I appreciate 
your patience.

> There are several places in the compiler that require these implicit copies 
> which aren’t just
>  normal expressions; this is the common pattern for handling them. The
>  synthesized expression can be emitted multiple times, and it can be freely
>  re-synthesized in different translation units instead of being serialized.

I'm not sure this is always the case. For example:

  // foo.h
  #include 
  
  struct Foo {
int x;
  };
  inline auto operator<=>(Foo const& LHS, Foo const& RHS) {
  
  }
  // foo.cpp
  #include  // imported via module.
  auto bar(Foo LHS, Foo RHS) {
return 
  }



> You’re already finding and caching a constructor; storing a
>  CXXConstructExpr is basically thr same thing, but in a nicer form that
>  handles more cases and doesn’t require as much redundant code in IRGen.

I'm not actually caching the copy constructor. And I disagree that storing a
`CXXConstructExpr` is essentially the same thing. I can lookup the 
`CXXConstructorDecl` without `Sema`,
but I can't build a `CXXConstructExpr` without it.

> STLs *frequently* make use of default arguments on copy constructors (for
>  allocators). I agree that it’s unlikely that that would happen here, but
>  that’s precisely because it’s unlikely that this type would ever be
>  non-trivial.
> 
> Mostly, though, I don’t understand the point of imposing a partial set of
>  non-conformant restrictions on the type. It’s easy to support an arbitrary
>  copy constructor by synthesizing a CXXConstructExpr, and this will
>  magically take care of any constexpr issues, as well as removing the need
>  for open-coding a constructor call.
> 
> The constexpr issues are that certain references to constexpr variables of
>  literal type (as these types are likely to be) are required to not ODR-use
>  the variable but instead just directly produce the initializer as the
>  expression result.  That’s especially important here because (1) existing
>  STL binaries will not define these variables, and we shouldn’t create
>  artificial deployment problems for this feature, and (2) we’d really rather
>  not emit these expressions as loads from externally-defined variables that
>  the optimizer won’t be able to optimize.
> 
> John.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[John McCall via cfe-commits]

I think you and Richard agreed that you weren’t going to synthesize a whole
expression tree at every use of the operator, and I agree with that
decision. That’s very different from what I’m asking you to do, which is to
synthesize in isolation a call to the copy-constructor. There are several
places in the compiler that require these implicit copies which aren’t just
normal expressions; this is the common pattern for handling them. The
synthesized expression can be emitted multiple times, and it can be freely
re-synthesized in different translation units instead of being serialized.
You’re already finding and caching a constructor; storing a
CXXConstructExpr is basically thr same thing, but in a nicer form that
handles more cases and doesn’t require as much redundant code in IRGen.

STLs *frequently* make use of default arguments on copy constructors (for
allocators). I agree that it’s unlikely that that would happen here, but
that’s precisely because it’s unlikely that this type would ever be
non-trivial.

Mostly, though, I don’t understand the point of imposing a partial set of
non-conformant restrictions on the type. It’s easy to support an arbitrary
copy constructor by synthesizing a CXXConstructExpr, and this will
magically take care of any constexpr issues, as well as removing the need
for open-coding a constructor call.

The constexpr issues are that certain references to constexpr variables of
literal type (as these types are likely to be) are required to not ODR-use
the variable but instead just directly produce the initializer as the
expression result.  That’s especially important here because (1) existing
STL binaries will not define these variables, and we shouldn’t create
artificial deployment problems for this feature, and (2) we’d really rather
not emit these expressions as loads from externally-defined variables that
the optimizer won’t be able to optimize.

John.

Eric Fiselier via Phabricator  wrote:

> EricWF added inline comments.
>
>
> 
> Comment at: lib/Sema/SemaDeclCXX.cpp:8931
> +  /*ConstArg*/ true, false, false, false, false);
> +  auto *CopyCtor = cast_or_null(SMI.getMethod());
> +
> 
> rjmccall wrote:
> > Sorry, I didn't realize you'd go off and write this code manually.
> >
> > The way we generally handle this sort of thing is just to synthesize an
> expression in Sema that performs the copy-construction.  That way, the
> stdlib can be as crazy as it wants — default arguments on the
> copy-constructor or whatever else — and it just works.  The pattern to
> follow here is the code in Sema::BuildExceptionDeclaration, except that in
> your case you can completely dispense with faking up an OpaqueValueExpr and
> instead just build a DeclRefExpr to the actual variable.  (You could even
> use ActOnIdExpression for this, although just synthesizing the DRE
> shouldn't be too hard.)  Then the ComparisonCategoryInfo can just store
> expressions for each of the three (four?) variables, and in IRGen you just
> evaluate the appropriate one into the destination.
> I think this goes against the direction Richard and I decided to go, which
> was to not synthesize any expressions.
>
> The problem is that the synthesized expressions cannot be stored in
> `ComparisonCategoryInfo`, because the data it contains is not serialized.
> So when we read the AST back, the `ComparisonCategoryInfo` will be empty.
> And for obvious reasons we can't cache the data in Sema either.
> Additionally, we probably don't want to waste space building and storing
> synthesized expressions in each AST node which needs them.
>
> Although the checking here leaves something to be desired, I suspect it's
> more than enough to handle any conforming STL implementation.
>
>
>
>
> https://reviews.llvm.org/D45476
>
>
>
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Mailing List "cfe-commits" via Phabricator via cfe-commits]

cfe-commits added a subscriber: junbuml.
cfe-commits added a comment.

I think you and Richard agreed that you weren’t going to synthesize a whole
expression tree at every use of the operator, and I agree with that
decision. That’s very different from what I’m asking you to do, which is to
synthesize in isolation a call to the copy-constructor. There are several
places in the compiler that require these implicit copies which aren’t just
normal expressions; this is the common pattern for handling them. The
synthesized expression can be emitted multiple times, and it can be freely
re-synthesized in different translation units instead of being serialized.
You’re already finding and caching a constructor; storing a
CXXConstructExpr is basically thr same thing, but in a nicer form that
handles more cases and doesn’t require as much redundant code in IRGen.

STLs *frequently* make use of default arguments on copy constructors (for
allocators). I agree that it’s unlikely that that would happen here, but
that’s precisely because it’s unlikely that this type would ever be
non-trivial.

Mostly, though, I don’t understand the point of imposing a partial set of
non-conformant restrictions on the type. It’s easy to support an arbitrary
copy constructor by synthesizing a CXXConstructExpr, and this will
magically take care of any constexpr issues, as well as removing the need
for open-coding a constructor call.

The constexpr issues are that certain references to constexpr variables of
literal type (as these types are likely to be) are required to not ODR-use
the variable but instead just directly produce the initializer as the
expression result.  That’s especially important here because (1) existing
STL binaries will not define these variables, and we shouldn’t create
artificial deployment problems for this feature, and (2) we’d really rather
not emit these expressions as loads from externally-defined variables that
the optimizer won’t be able to optimize.

John.

Eric Fiselier via Phabricator  wrote:

> EricWF added inline comments.
> 
> 
>  Comment at: lib/Sema/SemaDeclCXX.cpp:8931
>  +  /*ConstArg*/ true, false, false, false, false);
>  +  auto *CopyCtor = cast_or_null(SMI.getMethod());
>  +
> 
>  
> 
> rjmccall wrote:
>  > Sorry, I didn't realize you'd go off and write this code manually.
>  >
>  > The way we generally handle this sort of thing is just to synthesize an
>  expression in Sema that performs the copy-construction.  That way, the
>  stdlib can be as crazy as it wants — default arguments on the
>  copy-constructor or whatever else — and it just works.  The pattern to
>  follow here is the code in Sema::BuildExceptionDeclaration, except that in
>  your case you can completely dispense with faking up an OpaqueValueExpr and
>  instead just build a DeclRefExpr to the actual variable.  (You could even
>  use ActOnIdExpression for this, although just synthesizing the DRE
>  shouldn't be too hard.)  Then the ComparisonCategoryInfo can just store
>  expressions for each of the three (four?) variables, and in IRGen you just
>  evaluate the appropriate one into the destination.
>  I think this goes against the direction Richard and I decided to go, which
>  was to not synthesize any expressions.
> 
> The problem is that the synthesized expressions cannot be stored in
>  `ComparisonCategoryInfo`, because the data it contains is not serialized.
>  So when we read the AST back, the `ComparisonCategoryInfo` will be empty.
>  And for obvious reasons we can't cache the data in Sema either.
>  Additionally, we probably don't want to waste space building and storing
>  synthesized expressions in each AST node which needs them.
> 
> Although the checking here leaves something to be desired, I suspect it's
>  more than enough to handle any conforming STL implementation.
> 
> https://reviews.llvm.org/D45476



- F6099282: msg-9191-352.txt 


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: lib/Sema/SemaDeclCXX.cpp:8931
+  /*ConstArg*/ true, false, false, false, false);
+  auto *CopyCtor = cast_or_null(SMI.getMethod());
+

rjmccall wrote:
> Sorry, I didn't realize you'd go off and write this code manually.
> 
> The way we generally handle this sort of thing is just to synthesize an 
> expression in Sema that performs the copy-construction.  That way, the stdlib 
> can be as crazy as it wants — default arguments on the copy-constructor or 
> whatever else — and it just works.  The pattern to follow here is the code in 
> Sema::BuildExceptionDeclaration, except that in your case you can completely 
> dispense with faking up an OpaqueValueExpr and instead just build a 
> DeclRefExpr to the actual variable.  (You could even use ActOnIdExpression 
> for this, although just synthesizing the DRE shouldn't be too hard.)  Then 
> the ComparisonCategoryInfo can just store expressions for each of the three 
> (four?) variables, and in IRGen you just evaluate the appropriate one into 
> the destination.
I think this goes against the direction Richard and I decided to go, which was 
to not synthesize any expressions.

The problem is that the synthesized expressions cannot be stored in 
`ComparisonCategoryInfo`, because the data it contains is not serialized. So 
when we read the AST back, the `ComparisonCategoryInfo` will be empty. And for 
obvious reasons we can't cache the data in Sema either. Additionally, we 
probably don't want to waste space building and storing synthesized expressions 
in each AST node which needs them.

Although the checking here leaves something to be desired, I suspect it's more 
than enough to handle any conforming STL implementation.




https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/Sema/SemaDeclCXX.cpp:8931
+  /*ConstArg*/ true, false, false, false, false);
+  auto *CopyCtor = cast_or_null(SMI.getMethod());
+

Sorry, I didn't realize you'd go off and write this code manually.

The way we generally handle this sort of thing is just to synthesize an 
expression in Sema that performs the copy-construction.  That way, the stdlib 
can be as crazy as it wants — default arguments on the copy-constructor or 
whatever else — and it just works.  The pattern to follow here is the code in 
Sema::BuildExceptionDeclaration, except that in your case you can completely 
dispense with faking up an OpaqueValueExpr and instead just build a DeclRefExpr 
to the actual variable.  (You could even use ActOnIdExpression for this, 
although just synthesizing the DRE shouldn't be too hard.)  Then the 
ComparisonCategoryInfo can just store expressions for each of the three (four?) 
variables, and in IRGen you just evaluate the appropriate one into the 
destination.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: lib/CodeGen/CGExprAgg.cpp:1002
+  return EmitFinalDestCopy(
+  E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType()));
+}

EricWF wrote:
> rjmccall wrote:
> > Is there something in Sema actually validating that the comparison types is 
> > trivially copyable?  Because EmitFinalDestCopy does not work on non-trivial 
> > types.
> > 
> > Also, are we certain that we're allowed to do a copy from an actual global 
> > variable here instead of potentially a constexpr evaluation of the variable 
> > reference?  And if we are doing a copy, are we registering ODR-uses of all 
> > the variables in Sema?
> > 
> > I don't think you should worry about trying to generate a select between 
> > the "actual" comparison result values.  At least not yet.
> There is nothing is Sema validating that these comparison types are trivially 
> copyable, and according to the standard they don't need to be.
> I assumed we only ended up in `CGExprAgg` if the types were trivially 
> copyable. But I'll look into implementing this for non-trivially copyable 
> comparison types (although we'll probably never actually encounter them).
> 
> > Also, are we certain that we're allowed to do a copy from an actual global 
> > variable here instead of potentially a constexpr evaluation of the variable 
> > reference?
> 
> I'm not sure exactly what this means. How would I observe the difference?
> 
> >And if we are doing a copy, are we registering ODR-uses of all the variables 
> >in Sema?
> 
> Probably not. I'll double check that.
> 
> > I don't think you should worry about trying to generate a select between 
> > the "actual" comparison result values. At least not yet.
> 
> I'm not sure exactly what you mean by this.
To follow up:

>> And if we are doing a copy, are we registering ODR-uses of all the variables 
>> in Sema?
>
> Probably not. I'll double check that.

We do mark the potential result variables as ODR-used when we first check them 
when building builtin and defaulted comparison operators.

>> I don't think you should worry about trying to generate a select between the 
>> "actual" comparison result values. At least not yet.
>
> I'm not sure exactly what you mean by this.

Sorry, I see what you mean. You're talking about the comment. Richard asked me 
to leave that TODO there.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: lib/CodeGen/CGExprAgg.cpp:1002
+  return EmitFinalDestCopy(
+  E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType()));
+}

rjmccall wrote:
> Is there something in Sema actually validating that the comparison types is 
> trivially copyable?  Because EmitFinalDestCopy does not work on non-trivial 
> types.
> 
> Also, are we certain that we're allowed to do a copy from an actual global 
> variable here instead of potentially a constexpr evaluation of the variable 
> reference?  And if we are doing a copy, are we registering ODR-uses of all 
> the variables in Sema?
> 
> I don't think you should worry about trying to generate a select between the 
> "actual" comparison result values.  At least not yet.
There is nothing is Sema validating that these comparison types are trivially 
copyable, and according to the standard they don't need to be.
I assumed we only ended up in `CGExprAgg` if the types were trivially copyable. 
But I'll look into implementing this for non-trivially copyable comparison 
types (although we'll probably never actually encounter them).

> Also, are we certain that we're allowed to do a copy from an actual global 
> variable here instead of potentially a constexpr evaluation of the variable 
> reference?

I'm not sure exactly what this means. How would I observe the difference?

>And if we are doing a copy, are we registering ODR-uses of all the variables 
>in Sema?

Probably not. I'll double check that.

> I don't think you should worry about trying to generate a select between the 
> "actual" comparison result values. At least not yet.

I'm not sure exactly what you mean by this.



Comment at: lib/Sema/SemaExpr.cpp:9795
+  if (int Count = LHSType->isBooleanType() + RHSType->isBooleanType()) {
+// TODO: What about bool non-narrowing cases? Like '0' or '1.
+if (Count != 2) {

rsmith wrote:
> Missing `'`. Seems like a question to take to Herb and CWG (and maybe EWG), 
> but I suspect the answer will be "this does what we wanted".
> 
> Looks like P0946R0 missed this case of a difference between `<=>` and other 
> operators... oops.
I'll remove the comment for now then.



Comment at: lib/Sema/SemaExpr.cpp:9906
+  bool IsThreeWay = Opc == BO_Cmp;
+  auto IsPointerType = [](ExprResult E) {
+QualType Ty = E.get()->getType().getNonReferenceType();

rsmith wrote:
> I'd prefer a name that captures that this also recognizes member pointer 
> types.
I'm bad at naming things. Is `IsAnyPointerType` better?


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.



Comment at: lib/CodeGen/CGExprAgg.cpp:1002
+  return EmitFinalDestCopy(
+  E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType()));
+}

Is there something in Sema actually validating that the comparison types is 
trivially copyable?  Because EmitFinalDestCopy does not work on non-trivial 
types.

Also, are we certain that we're allowed to do a copy from an actual global 
variable here instead of potentially a constexpr evaluation of the variable 
reference?  And if we are doing a copy, are we registering ODR-uses of all the 
variables in Sema?

I don't think you should worry about trying to generate a select between the 
"actual" comparison result values.  At least not yet.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:9816
+RHS = S.ImpCastExprToType(RHS.get(), Type, CK_BitCast);
+  } else {
+// C++2a [expr.spaceship]p4

EricWF wrote:
> rsmith wrote:
> > We still need to apply the usual arithmetic conversions after converting 
> > enumerations to their underlying types (eg, `<=>` on `enum E : char` 
> > converts the operands first to `char` then to `int`). You could remove the 
> > `else` here and make this stuff unconditional, but it's probably better to 
> > sidestep the extra work and convert directly to the promoted type of the 
> > enum's underlying type.
> Do we still do usual arithmetic conversions if we have two enumerations of 
> the same type?
Formally, yes: "If both operands have the same enumeration type E, the operator 
yields the result of converting the operands to the underlying type of E and 
applying <=> to the converted operands."

The recursive application of `<=>` to the converted operands will perform the 
usual arithmetic conversions.



Comment at: lib/Sema/SemaExpr.cpp:9794
+  // other is not, the program is ill-formed.
+  if (int Count = LHSType->isBooleanType() + RHSType->isBooleanType()) {
+// TODO: What about bool non-narrowing cases? Like '0' or '1.

You could simplify this to `if (LHSType->isBooleanType() != 
RHSType->isBooleanType()) InvalidOperands`.



Comment at: lib/Sema/SemaExpr.cpp:9795
+  if (int Count = LHSType->isBooleanType() + RHSType->isBooleanType()) {
+// TODO: What about bool non-narrowing cases? Like '0' or '1.
+if (Count != 2) {

Missing `'`. Seems like a question to take to Herb and CWG (and maybe EWG), but 
I suspect the answer will be "this does what we wanted".

Looks like P0946R0 missed this case of a difference between `<=>` and other 
operators... oops.



Comment at: lib/Sema/SemaExpr.cpp:9829-9832
+if (EDecl->isScoped()) {
+  S.InvalidOperands(Loc, LHS, RHS);
+  return QualType();
+}

I don't think you need this check: the usual arithmetic conversions will fail 
to find a common type if the enum is scoped.



Comment at: lib/Sema/SemaExpr.cpp:9881-9890
   enum { StrongEquality, PartialOrdering, StrongOrdering } Ordering;
   if (Type->isAnyComplexType())
 Ordering = StrongEquality;
   else if (Type->isFloatingType())
 Ordering = PartialOrdering;
   else
 Ordering = StrongOrdering;

This is now over-complicated, since this is unreachable for `BO_Cmp`.



Comment at: lib/Sema/SemaExpr.cpp:9906
+  bool IsThreeWay = Opc == BO_Cmp;
+  auto IsPointerType = [](ExprResult E) {
+QualType Ty = E.get()->getType().getNonReferenceType();

I'd prefer a name that captures that this also recognizes member pointer types.



Comment at: lib/Sema/SemaExpr.cpp:9907
+  auto IsPointerType = [](ExprResult E) {
+QualType Ty = E.get()->getType().getNonReferenceType();
+return Ty->isPointerType() || Ty->isMemberPointerType();

You don't need a `.getNonReferenceType()` here; expressions can't have 
reference type.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 3 inline comments as done.
EricWF added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:9816
+RHS = S.ImpCastExprToType(RHS.get(), Type, CK_BitCast);
+  } else {
+// C++2a [expr.spaceship]p4

rsmith wrote:
> We still need to apply the usual arithmetic conversions after converting 
> enumerations to their underlying types (eg, `<=>` on `enum E : char` converts 
> the operands first to `char` then to `int`). You could remove the `else` here 
> and make this stuff unconditional, but it's probably better to sidestep the 
> extra work and convert directly to the promoted type of the enum's underlying 
> type.
Do we still do usual arithmetic conversions if we have two enumerations of the 
same type?


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:9928
+// result is of type std::strong_equality
+if (CompositeTy->isFunctionPointerType() ||
+CompositeTy->isMemberPointerType() || CompositeTy->isNullPtrType())

EricWF wrote:
> rsmith wrote:
> > Please add a FIXME here to consider making the function pointer case 
> > produce strong_ordering not strong_equality, per P0946R0-Jax18 discussion / 
> > direction polls.
> Is it OK if I go ahead and implement it anyway?
Nevermind. The changes seems involved enough they should be done separately. 


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 4 inline comments as done.
EricWF added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:9928
+// result is of type std::strong_equality
+if (CompositeTy->isFunctionPointerType() ||
+CompositeTy->isMemberPointerType() || CompositeTy->isNullPtrType())

rsmith wrote:
> Please add a FIXME here to consider making the function pointer case produce 
> strong_ordering not strong_equality, per P0946R0-Jax18 discussion / direction 
> polls.
Is it OK if I go ahead and implement it anyway?


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 24 inline comments as done.
EricWF added inline comments.



Comment at: lib/AST/ExprConstant.cpp:3784
+static bool EvaluateVarDecl(EvalInfo , const VarDecl *VD,
+APValue *Dest = nullptr) {
   // We don't need to evaluate the initializer for a static local.

rsmith wrote:
> This `Dest` parameter seems to be unused, is it left behind from a previous 
> direction?
Woops. It was, thanks!



Comment at: lib/CodeGen/CGExprAgg.cpp:1000-1001
+
+  return EmitFinalDestCopy(
+  E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType()));
+}

rsmith wrote:
> Hm, I wonder whether it's worthwhile to try to generate a select between the 
> comparison result values rather than their addresses. (Maybe not, since they 
> could in general be an arbitrary aggregate type, and a select on a 
> first-class aggregate value is unlikely to produce anything useful at -O0).
I was thinking something similar, but like you said, the STL implementation 
could provide an arbitrary aggregate type. 
However, I think that's a candidate for a follow up patch, so I'll add a `TODO` 
about it and leave it for now.




Comment at: lib/Sema/SemaExpr.cpp:9727-9728
+
+static bool checkNarrowingConversion(Sema , QualType ToType, Expr *E,
+ QualType FromType, SourceLocation Loc) {
+  // Check for a narrowing implicit conversion.

rsmith wrote:
> This should have a name that has something to do with three-way comparisons 
> (that is, assuming that duplicating this is the best way to customize the 
> diagnostic behavior).
I'm not sure this is the cleanest way to do it, but it seems better than trying 
to integrate it more directly with the `CheckConvertedConstantExpression` 
machinery. The semantics for `operator<=>` seems just different enough.

That being said, I'm very open to suggestions. You're the expert and resident 
compiler wizard.



Comment at: lib/Sema/SemaExpr.cpp:9807-9810
+if (!S.Context.hasSameUnqualifiedType(LHSType, RHSType)) {
+  S.InvalidOperands(Loc, LHS, RHS);
+  return QualType();
+}

rsmith wrote:
> Please implement the "straight to CWG" resolutions from 
> http://wiki.edg.com/bin/view/Wg21jacksonville2018/P0946R0-Jax18 directly 
> here. Specifically, in this case, we should allow three-way comparisons 
> between unscoped enumerations and integral types (subject to the narrowing 
> check), but not between two unrelated enumeration types, and not between a 
> scoped enumeration and an integral type.
I was thinking that if there wasn't already an issue for this behavior, there 
should be. Thanks for pointing it out.



Comment at: lib/Sema/SemaExpr.cpp:11942
 ConvertHalfVec = true;
 ResultTy = CheckCompareOperands(LHS, RHS, OpLoc, Opc, true);
+assert(ResultTy.isNull() || ResultTy->getAsCXXRecordDecl());

rsmith wrote:
> Ah, here it is, `true` is incorrectly being passed for `IsRelational` here. 
> Maybe replace that `bool` with an `enum` (or remove it entirely and have the 
> callee recompute it from `Opc`)?
Ack. Removing the parameter and re-computing it from `Opc`.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: lib/Sema/SemaExpr.cpp:9928
+// result is of type std::strong_equality
+if (CompositeTy->isFunctionPointerType() ||
+CompositeTy->isMemberPointerType() || CompositeTy->isNullPtrType())

Please add a FIXME here to consider making the function pointer case produce 
strong_ordering not strong_equality, per P0946R0-Jax18 discussion / direction 
polls.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks! This is looking pretty close.




Comment at: include/clang/AST/ComparisonCategories.h:78
+public:
+  /// \brief Wether Sema has fully checked the type and result values for this
+  ///   comparison category types before.

Typo "Wether".



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9377
+def err_implied_comparison_category_type_not_found : Error<
+  "cannot deduce return type of operator<=> because type %0 was not found; "
+  "include ">;

Nit: you have inconsistent quoting of 'operator<=>' between this diagnostic and 
the next.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9387
+  "standard library implementation of comparison category %0 is not supported; 
"
+   "failed to build reference to member '%1'">;
 } // end of sema component.

This seems a bit too implementation-detaily -- how about something vaguer like 
"[...] not supported; member '%1' does not have expected form"?

(I suppose it doesn't matter too much; only standard library implementers are 
likely to ever see this diagnostic anyway.)



Comment at: lib/AST/ComparisonCategories.cpp:51
+if (!StdNS) {
+  StdNS = NamespaceDecl::Create(
+  const_cast(Ctx), Ctx.getTranslationUnitDecl(),

We shouldn't be creating a 'namespace std' here. If there is no existing such 
namespace, our lookups should just fail.



Comment at: lib/AST/ExprConstant.cpp:3784
+static bool EvaluateVarDecl(EvalInfo , const VarDecl *VD,
+APValue *Dest = nullptr) {
   // We don't need to evaluate the initializer for a static local.

This `Dest` parameter seems to be unused, is it left behind from a previous 
direction?



Comment at: lib/AST/ExprConstant.cpp:8824
+  return EvaluateComparisonBinaryOperator(Info, E, OnSuccess, []() -> bool {
+llvm_unreachable("operator<=> should have been evaluated to a result");
+  });

I'm not convinced this is a legitimate use of `llvm_unreachable` -- it seems to 
me that there could be all sorts of types for which Sema can form a `<=>` 
expression but that we don't handle here, such as vector types. Maybe issue an 
`FFDiag` here, or just call the base class version of this function, which I 
think should do it for you?



Comment at: lib/CodeGen/CGExprAgg.cpp:947-954
+return CGF.ErrorUnsupported(
+E, "aggregate binary expression with complex arguments");
+  if (ArgTy->isVectorType())
+return CGF.ErrorUnsupported(
+E, "aggregate binary expression with vector arguments");
+  if (!ArgTy->isIntegralOrEnumerationType() && !ArgTy->isRealFloatingType() &&
+  !ArgTy->isPointerType() && !ArgTy->isMemberPointerType())

Instead of "aggregate binary expression" in all of these errors, how about 
"three-way comparison"?



Comment at: lib/CodeGen/CGExprAgg.cpp:1000-1001
+
+  return EmitFinalDestCopy(
+  E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType()));
+}

Hm, I wonder whether it's worthwhile to try to generate a select between the 
comparison result values rather than their addresses. (Maybe not, since they 
could in general be an arbitrary aggregate type, and a select on a first-class 
aggregate value is unlikely to produce anything useful at -O0).



Comment at: lib/Sema/SemaDeclCXX.cpp:8903-8916
+  // Build the initial category information
+  RecordDecl *CCDecl = nullptr;
+  // Lookup the record for the category type
+  if (auto Std = getStdNamespace()) {
+LookupResult Result(*this, ().get(Name),
+SourceLocation(), Sema::LookupTagName);
+if (LookupQualifiedName(Result, Std))

I don't think this makes sense: `CompCategories::lookupInfo` already did the 
lookup we wanted here; checking some other declaration just invites the two 
lookups being inconsistent in some way. I think you should just check whether 
`CachedInfo` is null here, and if so, produce the "type not found" error.



Comment at: lib/Sema/SemaDeclCXX.cpp:8936-8948
+  // Build each of the require values and store them in Info.
+  for (CCVT CCV : Values) {
+StringRef ValueName = ComparisonCategories::getResultString(CCV);
+QualType Ty(CCDecl->getTypeForDecl(), 0);
+DeclContext *LookupCtx = computeDeclContext(Ty);
+LookupResult Found(*this, ().get(ValueName), Loc,
+   Sema::LookupOrdinaryName);

Likewise here, you should query the `CachedInfo` object for these values and 
check them, rather than looking them up again in a different way.



Comment at: lib/Sema/SemaDeclCXX.cpp:8966
+  assert(CachedInfo->Kind == Kind);
+  CachedInfo->IsFullyChecked = true;
+  return QualType(CachedInfo->CCDecl->getTypeForDecl(), 0);

I would 

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 30 inline comments as done.
EricWF added inline comments.



Comment at: include/clang/AST/ComparisonCategories.h:194
+
+  const ASTContext 
+  mutable llvm::DenseMap Data;

Storing the `ASTContext` in `ComparisonCategories` and `ComparisonCategoryInfo` 
is a bit of a hack. I'm going to work on either passing `ASTContext`  as 
needed, or integrating these types directly with `ASTContext`



Comment at: include/clang/AST/ComparisonCategories.h:77-78
+  ///   comparison category. For example 'std::strong_equality::equal'
+  const DeclRefExpr *getResultValue(ComparisonCategoryResult ValueKind) const {
+const DeclRefExpr *DR = getResultValueUnsafe(ValueKind);
+assert(DR &&

rsmith wrote:
> EricWF wrote:
> > rsmith wrote:
> > > EricWF wrote:
> > > > rsmith wrote:
> > > > > This should deal in `DeclRefExpr*`s, not `NamedDecl*`s. We don't have 
> > > > > an expression naming the comparison result value, and we shouldn't 
> > > > > pretend we do.
> > > > I'm confused. This does deal in `DeclRefExpr*`s. I'm probably being 
> > > > daft. Could you clarify?
> > > Sorry, while editing this comment I managed to reverse it from what I 
> > > meant. This should deal in NamedDecl* (or perhaps ValueDecl*) , not 
> > > DeclRefExpr*.
> > OK, so I tried changing this to `VarDecl` but it made the `ExprConstant` 
> > and `GCExprAgg` implementations a lot harder, since we actually want to 
> > evaluate the result as a `DeclRefExpr` as we're not evaluating the actual 
> > `Decl`. 
> > 
> > Since we don't want to be building dummy `DeclRefExpr`s during evaluation 
> > just to throw them away, I think it makes sense to eagerly build the 
> > results as `DeclRefExpr`s.
> > 
> > Does that make sense?
> Not really; forming a value that denotes an arbitrary (non-reference) 
> variable in both those places seems like it should be straightforward. If you 
> really want a DeclRefExpr in those places as an implementation convenience, 
> you could synthesize one on the stack. But that'd be an implementation detail 
> of those consumers of this information, not something we should be exposing 
> here, and seems like it should be easy to avoid.
Ack. Done. I'm not sure why CodeGen gave me so much trouble. But you were 
right, it ended up being straight forward.



Comment at: lib/Sema/SemaDeclCXX.cpp:
 
+bool Sema::BuildComparisonCategoryData(SourceLocation Loc) {
+  using CCKT = ComparisonCategoryKind;

rsmith wrote:
> EricWF wrote:
> > rsmith wrote:
> > > EricWF wrote:
> > > > rsmith wrote:
> > > > > If you put this on Sema, you'll need to serialize it, and it's no 
> > > > > longer just a cache.
> > > > > 
> > > > > I would prefer to treat this information strictly as a cache, and 
> > > > > build it in ASTContext. Sema should then just be checking that the 
> > > > > information is "valid" when it first makes use of such a comparison 
> > > > > category type.
> > > > I agree that it's preferable to just have a cache. However can you 
> > > > clarify what you mean by "build it in ASTContext"?
> > > > 
> > > > Building the expressions will potentially require emitting a 
> > > > diagnostic, and use of bits of `Sema` (like name lookup).
> > > > I'm not sure how to do this inside `ASTContext`.
> > > > 
> > > > The current implementation caches the data in `ASTContext` but builds 
> > > > it as-needed in `Sema`. Could you clarify how to go about with the 
> > > > implementation you're suggesting?
> > > You don't need "real" name lookup here, just calling lookup on the 
> > > DeclContext should be fine. We don't need to support more exotic ways of 
> > > declaring these members, nor access checks etc.
> > > 
> > > I was imagining that Sema would check that we can find suitable members 
> > > on the comparison category types as part of semantically checking a <=> 
> > > expression, and the ASTContext side of things would not emit diagnostics.
> > > 
> > > (Put another way, we'd act as if we look up the members each time we need 
> > > them, Sema would check that all the necessary members actually exist, and 
> > > ASTContext would have a caching layer only for convenience / to avoid 
> > > repeatedly doing the same lookups.)
> > That's pretty much how this currently works, right?
> > 
> > The only difference is that Sema still eagerly builds the potential result 
> > values eagerly, because they'll be needed later.
> You can't rely on Sema existing or having done this in the case where we're 
> loading from an AST file.
Ack. Thanks for sticking with me. I understand now.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/AST/ComparisonCategories.h:77-78
+  ///   comparison category. For example 'std::strong_equality::equal'
+  const DeclRefExpr *getResultValue(ComparisonCategoryResult ValueKind) const {
+const DeclRefExpr *DR = getResultValueUnsafe(ValueKind);
+assert(DR &&

EricWF wrote:
> rsmith wrote:
> > EricWF wrote:
> > > rsmith wrote:
> > > > This should deal in `DeclRefExpr*`s, not `NamedDecl*`s. We don't have 
> > > > an expression naming the comparison result value, and we shouldn't 
> > > > pretend we do.
> > > I'm confused. This does deal in `DeclRefExpr*`s. I'm probably being daft. 
> > > Could you clarify?
> > Sorry, while editing this comment I managed to reverse it from what I 
> > meant. This should deal in NamedDecl* (or perhaps ValueDecl*) , not 
> > DeclRefExpr*.
> OK, so I tried changing this to `VarDecl` but it made the `ExprConstant` and 
> `GCExprAgg` implementations a lot harder, since we actually want to evaluate 
> the result as a `DeclRefExpr` as we're not evaluating the actual `Decl`. 
> 
> Since we don't want to be building dummy `DeclRefExpr`s during evaluation 
> just to throw them away, I think it makes sense to eagerly build the results 
> as `DeclRefExpr`s.
> 
> Does that make sense?
Not really; forming a value that denotes an arbitrary (non-reference) variable 
in both those places seems like it should be straightforward. If you really 
want a DeclRefExpr in those places as an implementation convenience, you could 
synthesize one on the stack. But that'd be an implementation detail of those 
consumers of this information, not something we should be exposing here, and 
seems like it should be easy to avoid.



Comment at: lib/Sema/SemaDeclCXX.cpp:
 
+bool Sema::BuildComparisonCategoryData(SourceLocation Loc) {
+  using CCKT = ComparisonCategoryKind;

EricWF wrote:
> rsmith wrote:
> > EricWF wrote:
> > > rsmith wrote:
> > > > If you put this on Sema, you'll need to serialize it, and it's no 
> > > > longer just a cache.
> > > > 
> > > > I would prefer to treat this information strictly as a cache, and build 
> > > > it in ASTContext. Sema should then just be checking that the 
> > > > information is "valid" when it first makes use of such a comparison 
> > > > category type.
> > > I agree that it's preferable to just have a cache. However can you 
> > > clarify what you mean by "build it in ASTContext"?
> > > 
> > > Building the expressions will potentially require emitting a diagnostic, 
> > > and use of bits of `Sema` (like name lookup).
> > > I'm not sure how to do this inside `ASTContext`.
> > > 
> > > The current implementation caches the data in `ASTContext` but builds it 
> > > as-needed in `Sema`. Could you clarify how to go about with the 
> > > implementation you're suggesting?
> > You don't need "real" name lookup here, just calling lookup on the 
> > DeclContext should be fine. We don't need to support more exotic ways of 
> > declaring these members, nor access checks etc.
> > 
> > I was imagining that Sema would check that we can find suitable members on 
> > the comparison category types as part of semantically checking a <=> 
> > expression, and the ASTContext side of things would not emit diagnostics.
> > 
> > (Put another way, we'd act as if we look up the members each time we need 
> > them, Sema would check that all the necessary members actually exist, and 
> > ASTContext would have a caching layer only for convenience / to avoid 
> > repeatedly doing the same lookups.)
> That's pretty much how this currently works, right?
> 
> The only difference is that Sema still eagerly builds the potential result 
> values eagerly, because they'll be needed later.
You can't rely on Sema existing or having done this in the case where we're 
loading from an AST file.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: include/clang/AST/ComparisonCategories.h:77-78
+  ///   comparison category. For example 'std::strong_equality::equal'
+  const DeclRefExpr *getResultValue(ComparisonCategoryResult ValueKind) const {
+const DeclRefExpr *DR = getResultValueUnsafe(ValueKind);
+assert(DR &&

rsmith wrote:
> EricWF wrote:
> > rsmith wrote:
> > > This should deal in `DeclRefExpr*`s, not `NamedDecl*`s. We don't have an 
> > > expression naming the comparison result value, and we shouldn't pretend 
> > > we do.
> > I'm confused. This does deal in `DeclRefExpr*`s. I'm probably being daft. 
> > Could you clarify?
> Sorry, while editing this comment I managed to reverse it from what I meant. 
> This should deal in NamedDecl* (or perhaps ValueDecl*) , not DeclRefExpr*.
OK, so I tried changing this to `VarDecl` but it made the `ExprConstant` and 
`GCExprAgg` implementations a lot harder, since we actually want to evaluate 
the result as a `DeclRefExpr` as we're not evaluating the actual `Decl`. 

Since we don't want to be building dummy `DeclRefExpr`s during evaluation just 
to throw them away, I think it makes sense to eagerly build the results as 
`DeclRefExpr`s.

Does that make sense?


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: lib/Sema/SemaDeclCXX.cpp:
 
+bool Sema::BuildComparisonCategoryData(SourceLocation Loc) {
+  using CCKT = ComparisonCategoryKind;

rsmith wrote:
> EricWF wrote:
> > rsmith wrote:
> > > If you put this on Sema, you'll need to serialize it, and it's no longer 
> > > just a cache.
> > > 
> > > I would prefer to treat this information strictly as a cache, and build 
> > > it in ASTContext. Sema should then just be checking that the information 
> > > is "valid" when it first makes use of such a comparison category type.
> > I agree that it's preferable to just have a cache. However can you clarify 
> > what you mean by "build it in ASTContext"?
> > 
> > Building the expressions will potentially require emitting a diagnostic, 
> > and use of bits of `Sema` (like name lookup).
> > I'm not sure how to do this inside `ASTContext`.
> > 
> > The current implementation caches the data in `ASTContext` but builds it 
> > as-needed in `Sema`. Could you clarify how to go about with the 
> > implementation you're suggesting?
> You don't need "real" name lookup here, just calling lookup on the 
> DeclContext should be fine. We don't need to support more exotic ways of 
> declaring these members, nor access checks etc.
> 
> I was imagining that Sema would check that we can find suitable members on 
> the comparison category types as part of semantically checking a <=> 
> expression, and the ASTContext side of things would not emit diagnostics.
> 
> (Put another way, we'd act as if we look up the members each time we need 
> them, Sema would check that all the necessary members actually exist, and 
> ASTContext would have a caching layer only for convenience / to avoid 
> repeatedly doing the same lookups.)
That's pretty much how this currently works, right?

The only difference is that Sema still eagerly builds the potential result 
values eagerly, because they'll be needed later.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9384-9385
+def err_spaceship_comparison_of_void_ptr : Error<
+  "three-way comparison with void pointer %select{operand type|operand types}0 
"
+  "(%1 and %2)">;
+def err_spaceship_comparison_of_invalid_comp_type : Error<

rsmith wrote:
> EricWF wrote:
> > rsmith wrote:
> > > Why are you diagnosing this case as an error?
> > [expr.spaceship]p9 seems to require it. The composite pointer type is not a 
> > function/member/null pointer type, neither is it a object pointer type, so 
> > the program is ill-formed.
> > 
> > Unless I'm missing something.
> void* is an object pointer type.
Oh. right. 'object pointer' vs 'pointer-to-object'.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/AST/ComparisonCategories.h:77-78
+  ///   comparison category. For example 'std::strong_equality::equal'
+  const DeclRefExpr *getResultValue(ComparisonCategoryResult ValueKind) const {
+const DeclRefExpr *DR = getResultValueUnsafe(ValueKind);
+assert(DR &&

EricWF wrote:
> rsmith wrote:
> > This should deal in `DeclRefExpr*`s, not `NamedDecl*`s. We don't have an 
> > expression naming the comparison result value, and we shouldn't pretend we 
> > do.
> I'm confused. This does deal in `DeclRefExpr*`s. I'm probably being daft. 
> Could you clarify?
Sorry, while editing this comment I managed to reverse it from what I meant. 
This should deal in NamedDecl* (or perhaps ValueDecl*) , not DeclRefExpr*.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9384-9385
+def err_spaceship_comparison_of_void_ptr : Error<
+  "three-way comparison with void pointer %select{operand type|operand types}0 
"
+  "(%1 and %2)">;
+def err_spaceship_comparison_of_invalid_comp_type : Error<

EricWF wrote:
> rsmith wrote:
> > Why are you diagnosing this case as an error?
> [expr.spaceship]p9 seems to require it. The composite pointer type is not a 
> function/member/null pointer type, neither is it a object pointer type, so 
> the program is ill-formed.
> 
> Unless I'm missing something.
void* is an object pointer type.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9393
+def note_spaceship_operand_not_cce : Note<
+  "argument is not a constant expression">;
 } // end of sema component.

EricWF wrote:
> rsmith wrote:
> > Is this really useful? I would think almost all the cases where you'd hit 
> > the "cannot be narrowed" error, this diagnostic and the explanation of why 
> > the operand is not constant would be meaningless noise, because it was 
> > never meant to be constant, and the problem is simply that you are trying 
> > to three-way compare integers of mixed signedness.
> I'm struggling to answer that question myself. The case I was thinking of 
> that I wanted to help the user out with is:
> 
> ```
> auto cmp_sentinal(long val) {
>   int SENTINAL = 0;
>   return SENTINAL <=> val; // error, would be OK if SENTINAL were const.
> }
> ```
> 
> I'll remove these diagnostics for now, and hopefully improve them in a follow 
> up patch, if that's OK with you?
Sounds good.



Comment at: lib/Sema/SemaDeclCXX.cpp:
 
+bool Sema::BuildComparisonCategoryData(SourceLocation Loc) {
+  using CCKT = ComparisonCategoryKind;

EricWF wrote:
> rsmith wrote:
> > If you put this on Sema, you'll need to serialize it, and it's no longer 
> > just a cache.
> > 
> > I would prefer to treat this information strictly as a cache, and build it 
> > in ASTContext. Sema should then just be checking that the information is 
> > "valid" when it first makes use of such a comparison category type.
> I agree that it's preferable to just have a cache. However can you clarify 
> what you mean by "build it in ASTContext"?
> 
> Building the expressions will potentially require emitting a diagnostic, and 
> use of bits of `Sema` (like name lookup).
> I'm not sure how to do this inside `ASTContext`.
> 
> The current implementation caches the data in `ASTContext` but builds it 
> as-needed in `Sema`. Could you clarify how to go about with the 
> implementation you're suggesting?
You don't need "real" name lookup here, just calling lookup on the DeclContext 
should be fine. We don't need to support more exotic ways of declaring these 
members, nor access checks etc.

I was imagining that Sema would check that we can find suitable members on the 
comparison category types as part of semantically checking a <=> expression, 
and the ASTContext side of things would not emit diagnostics.

(Put another way, we'd act as if we look up the members each time we need them, 
Sema would check that all the necessary members actually exist, and ASTContext 
would have a caching layer only for convenience / to avoid repeatedly doing the 
same lookups.)


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 7 inline comments as done.
EricWF added inline comments.



Comment at: lib/AST/ExprConstant.cpp:6238-6263
+bool VisitBinCmp(const BinaryOperator *E) {
+  using CCR = ComparisonCategoryResult;
+  const ComparisonCategoryInfo  =
+  Info.Ctx.CompCategories.getInfoForType(E->getType());
+
+  // Build a new version of the binary operator which returns an integer
+  // representing the ComparisonCategoryResult. Then defer to

rsmith wrote:
> I don't like this approach (inventing an expression that is invalid so that 
> we can call into some other code to compute this result). If this is really 
> materially better than implementing `<=>` directly here, this should be 
> justified by comments here. But I would suspect that factoring out the common 
> code for dealing with the various kinds of comparison from `IntExprEvaluator` 
> and calling it from both here and there would lead to an overall better 
> design.
Ack.

I tried to split the code out, but it wasn't easy and the result wasn't pretty. 
I'll go ahead and give this another go.
The problems I ran into were:
* de-duplicating error checking and argument evaluation across comparison and 
non-comparison operations.
* three-way and non-three-way comparison operations still return a 
fundamentally different type, so reporting the results still requires either 
representing Cmp results as an integer, or having different mechanism to report 
the result.

Any advice or input you have would be appreciated.


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 5 inline comments as done.
EricWF added inline comments.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9377-9378
+def err_implied_comparison_category_type_not_found : Error<
+  "%0 type was not found; include  before defining "
+  "or using 'operator<=>'">;
+def err_spaceship_argument_narrowing : Error<

rsmith wrote:
> This doesn't sound quite right. You can define your own `operator<=>` without 
> including `` so long as you don't try to default it or use one of 
> the standard comparison category kinds. Also, instead of saying "before doing 
> X or Y", say which one the user actually did.
Ah, right. I guess this should say "cannot deduce return type of operator<=> 
because type %0 was not found; include "



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9384-9385
+def err_spaceship_comparison_of_void_ptr : Error<
+  "three-way comparison with void pointer %select{operand type|operand types}0 
"
+  "(%1 and %2)">;
+def err_spaceship_comparison_of_invalid_comp_type : Error<

rsmith wrote:
> Why are you diagnosing this case as an error?
[expr.spaceship]p9 seems to require it. The composite pointer type is not a 
function/member/null pointer type, neither is it a object pointer type, so the 
program is ill-formed.

Unless I'm missing something.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9391
+def err_std_compare_type_invalid_member : Error<
+  "member '%1' of %0 is ill-formed">;
+def note_spaceship_operand_not_cce : Note<

rsmith wrote:
> What do you mean by "ill-formed" here?
> 
> I think what you really mean is "we don't understand how to deal with this 
> definition of that member", so maybe we should just fold this and the prior 
> diagnostic into something like "sorry, this standard library implementation 
> of std::whatever is not supported"?
That's exactly what I'm trying to say. 'll clean the diagnostic up. Thanks!



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9393
+def note_spaceship_operand_not_cce : Note<
+  "argument is not a constant expression">;
 } // end of sema component.

rsmith wrote:
> Is this really useful? I would think almost all the cases where you'd hit the 
> "cannot be narrowed" error, this diagnostic and the explanation of why the 
> operand is not constant would be meaningless noise, because it was never 
> meant to be constant, and the problem is simply that you are trying to 
> three-way compare integers of mixed signedness.
I'm struggling to answer that question myself. The case I was thinking of that 
I wanted to help the user out with is:

```
auto cmp_sentinal(long val) {
  int SENTINAL = 0;
  return SENTINAL <=> val; // error, would be OK if SENTINAL were const.
}
```

I'll remove these diagnostics for now, and hopefully improve them in a follow 
up patch, if that's OK with you?


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 4 inline comments as done.
EricWF added inline comments.



Comment at: include/clang/AST/Expr.h:3097-3106
+  bool isRelationalOp() const {
+return isRelationalOp(getOpcode()) ||
+   (getOpcode() == BO_Cmp && IsCmpOrdered);
+  }
 
   static bool isEqualityOp(Opcode Opc) { return Opc == BO_EQ || Opc == BO_NE; }
+  bool isEqualityOp() const {

rsmith wrote:
> These seem wrong to me. Relational operators are `<`, `<=`, `>`, and `>=`. 
> `<=>` is not a relational operator even if it's an ordered comparison. 
> Likewise, `<=>` is not an equality operator even if we don't have an ordered 
> comparison.
> 
> If you undo this, you can also drop the `IsCmpOrdered` member entirely.
OK. I'll find another way to do this.

The intention was to facilitate identification of three-way comparisons which 
require equality/relational comparisons, which may or may not be legal in 
certain contexts. For example, comparing member pointers of different bases is 
OK, but only so long as we're evaluating them for strong/weak equality. 


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF marked 4 inline comments as done.
EricWF added inline comments.



Comment at: include/clang/AST/ComparisonCategories.h:68
+  /// \brief A map containing the comparison category values built from the
+  /// standard library. The key is a value of ComparisonCategoryValue.
+  llvm::DenseMap Objects;

rsmith wrote:
> What is ComparisonCategoryValue? Do you mean ComparisonCategoryResult?
Yes. Fixed.



Comment at: include/clang/AST/ComparisonCategories.h:77-78
+  ///   comparison category. For example 'std::strong_equality::equal'
+  const DeclRefExpr *getResultValue(ComparisonCategoryResult ValueKind) const {
+const DeclRefExpr *DR = getResultValueUnsafe(ValueKind);
+assert(DR &&

rsmith wrote:
> This should deal in `DeclRefExpr*`s, not `NamedDecl*`s. We don't have an 
> expression naming the comparison result value, and we shouldn't pretend we do.
I'm confused. This does deal in `DeclRefExpr*`s. I'm probably being daft. Could 
you clarify?



Comment at: lib/AST/ExprConstant.cpp:7107
   IntExprEvaluator(EvalInfo , APValue )
-: ExprEvaluatorBaseTy(info), Result(result) {}
+  : ExprEvaluatorBase(info), Result(result) {}
 

rsmith wrote:
> Historically MSVC has choked on use of the injected-class-name of a base 
> class that is a template specialization. Is this fixed in our supported 
> versions now?
No idea. I didn't mean to change it (I had a different version that did).

I'll correct it.



Comment at: lib/Sema/SemaDeclCXX.cpp:
 
+bool Sema::BuildComparisonCategoryData(SourceLocation Loc) {
+  using CCKT = ComparisonCategoryKind;

rsmith wrote:
> If you put this on Sema, you'll need to serialize it, and it's no longer just 
> a cache.
> 
> I would prefer to treat this information strictly as a cache, and build it in 
> ASTContext. Sema should then just be checking that the information is "valid" 
> when it first makes use of such a comparison category type.
I agree that it's preferable to just have a cache. However can you clarify what 
you mean by "build it in ASTContext"?

Building the expressions will potentially require emitting a diagnostic, and 
use of bits of `Sema` (like name lookup).
I'm not sure how to do this inside `ASTContext`.

The current implementation caches the data in `ASTContext` but builds it 
as-needed in `Sema`. Could you clarify how to go about with the implementation 
you're suggesting?


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: include/clang/AST/ComparisonCategories.h:68
+  /// \brief A map containing the comparison category values built from the
+  /// standard library. The key is a value of ComparisonCategoryValue.
+  llvm::DenseMap Objects;

What is ComparisonCategoryValue? Do you mean ComparisonCategoryResult?



Comment at: include/clang/AST/ComparisonCategories.h:77-78
+  ///   comparison category. For example 'std::strong_equality::equal'
+  const DeclRefExpr *getResultValue(ComparisonCategoryResult ValueKind) const {
+const DeclRefExpr *DR = getResultValueUnsafe(ValueKind);
+assert(DR &&

This should deal in `DeclRefExpr*`s, not `NamedDecl*`s. We don't have an 
expression naming the comparison result value, and we shouldn't pretend we do.



Comment at: include/clang/AST/ComparisonCategories.h:175-177
+  /// \brief returns true if the comparison category data has already been
+  /// built, and false otherwise.
+  bool hasData() const { return HasData; }

I don't think this should be exposed. (I think we should only be building the 
CCK that we need when we request them, rather than building all five, and if we 
do that, then this function is meaningless.)



Comment at: include/clang/AST/ComparisonCategories.h:180
+public:
+  // implementation details which should only be used by the function creating
+  // and setting the data.

Comments should start with a capital letter.



Comment at: include/clang/AST/Expr.h:3097-3106
+  bool isRelationalOp() const {
+return isRelationalOp(getOpcode()) ||
+   (getOpcode() == BO_Cmp && IsCmpOrdered);
+  }
 
   static bool isEqualityOp(Opcode Opc) { return Opc == BO_EQ || Opc == BO_NE; }
+  bool isEqualityOp() const {

These seem wrong to me. Relational operators are `<`, `<=`, `>`, and `>=`. 
`<=>` is not a relational operator even if it's an ordered comparison. 
Likewise, `<=>` is not an equality operator even if we don't have an ordered 
comparison.

If you undo this, you can also drop the `IsCmpOrdered` member entirely.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9377-9378
+def err_implied_comparison_category_type_not_found : Error<
+  "%0 type was not found; include  before defining "
+  "or using 'operator<=>'">;
+def err_spaceship_argument_narrowing : Error<

This doesn't sound quite right. You can define your own `operator<=>` without 
including `` so long as you don't try to default it or use one of the 
standard comparison category kinds. Also, instead of saying "before doing X or 
Y", say which one the user actually did.



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9384-9385
+def err_spaceship_comparison_of_void_ptr : Error<
+  "three-way comparison with void pointer %select{operand type|operand types}0 
"
+  "(%1 and %2)">;
+def err_spaceship_comparison_of_invalid_comp_type : Error<

Why are you diagnosing this case as an error?



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9386-9387
+  "(%1 and %2)">;
+def err_spaceship_comparison_of_invalid_comp_type : Error<
+  "three-way comparison of operands has composite type %0 (%1 and %2)">;
+def err_std_compare_type_missing_member : Error<

It's unclear to me what this diagnostic is supposed to mean. What actionable 
feedback is this supposed to be giving a user?



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9389
+def err_std_compare_type_missing_member : Error<
+  "%0 missing a member named '%1'">;
+def err_std_compare_type_invalid_member : Error<

"%0 *is* missing [...]"?



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9391
+def err_std_compare_type_invalid_member : Error<
+  "member '%1' of %0 is ill-formed">;
+def note_spaceship_operand_not_cce : Note<

What do you mean by "ill-formed" here?

I think what you really mean is "we don't understand how to deal with this 
definition of that member", so maybe we should just fold this and the prior 
diagnostic into something like "sorry, this standard library implementation of 
std::whatever is not supported"?



Comment at: include/clang/Basic/DiagnosticSemaKinds.td:9393
+def note_spaceship_operand_not_cce : Note<
+  "argument is not a constant expression">;
 } // end of sema component.

Is this really useful? I would think almost all the cases where you'd hit the 
"cannot be narrowed" error, this diagnostic and the explanation of why the 
operand is not constant would be meaningless noise, because it was never meant 
to be constant, and the problem is simply that you are trying to three-way 
compare integers of mixed signedness.



Comment at: 

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.



Comment at: include/clang/AST/ComparisonCategories.h:155-158
+  const ComparisonCategoryInfo (ComparisonCategoryKind Kind) const {
+assert(HasData && "comparison category information not yet built");
+return Data[static_cast(Kind)];
+  }

aaron.ballman wrote:
> It seems like this method can be private rather than public.
`getInfo`? No that's used elsewhere.



Comment at: include/clang/AST/Expr.h:3020
+  // ordered comparison. Otherwise the comparison is an equality comparison.
+  unsigned IsCmpOrdered : 1;
+

aaron.ballman wrote:
> It's unfortunate that this means we're using 9 bits rather than the 8 we were 
> previously using, but I don't see a particularly good way to solve that.
It doesn't seem to be an actual problem. That is, it doesn't change the size of 
`BinaryOperator`. Even sinking the bits into a new Stmt bitfield class has no 
effect, even though there is room for them there.



Comment at: lib/CodeGen/CGExprAgg.cpp:925-929
+  } else if (ArgTy->isIntegralOrEnumerationType() || ArgTy->isPointerType()) {
+auto Inst =
+ArgTy->hasSignedIntegerRepresentation() ? InstInfo.SCmp : 
InstInfo.UCmp;
+return Builder.CreateICmp(Inst, LHS, RHS, "cmp");
+  } else if (ArgTy->isAnyComplexType()) {

aaron.ballman wrote:
> You can remove the `else` after a return and just turn these into `if` 
> statements.
I kind of like the  unimplemented cases being chained via `else if` to the 
implemented ones. It implies that "these are all the cases there are and 
they're mutually exclusive".


https://reviews.llvm.org/D45476



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D45476: [C++2a] Implement operator<=> CodeGen and ExprConstant

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this!




Comment at: include/clang/AST/ComparisonCategories.h:78
+  const DeclRefExpr *getResultValue(ComparisonCategoryResult ValueKind) const {
+const auto *DR = getResultValueUnsafe(ValueKind);
+assert(DR &&

Avoid `auto` when the type is not explicitly spelled out in the initialization.



Comment at: include/clang/AST/ComparisonCategories.h:155-158
+  const ComparisonCategoryInfo (ComparisonCategoryKind Kind) const {
+assert(HasData && "comparison category information not yet built");
+return Data[static_cast(Kind)];
+  }

It seems like this method can be private rather than public.



Comment at: include/clang/AST/ComparisonCategories.h:170
+
+  /// \brief Return the comparison category kind coorisponding to the specified
+  ///   type. 'Ty' is expected to refer to the type of one of the comparison

s/coorisponding/corresponding



Comment at: include/clang/AST/ComparisonCategories.h:176-177
+  /// \brief returns true if the comparison category data has already been
+  /// built,
+  ///and false otherwise.
+  bool hasData() const { return HasData; }

Did this get re-flowed in a strange manner?



Comment at: include/clang/AST/ComparisonCategories.h:180
+
+public: // private
+  using InfoList =

The comment here seems incorrect.



Comment at: include/clang/AST/Expr.h:3020
+  // ordered comparison. Otherwise the comparison is an equality comparison.
+  unsigned IsCmpOrdered : 1;
+

It's unfortunate that this means we're using 9 bits rather than the 8 we were 
previously using, but I don't see a particularly good way to solve that.



Comment at: include/clang/AST/Expr.h:3028
   BinaryOperator(Expr *lhs, Expr *rhs, Opcode opc, QualType ResTy,
- ExprValueKind VK, ExprObjectKind OK,
- SourceLocation opLoc, FPOptions FPFeatures)
+ ExprValueKind VK, ExprObjectKind OK, SourceLocation opLoc,
+ FPOptions FPFeatures)

s/opLoc/OpLoc



Comment at: include/clang/AST/Expr.h:3167
 
+  void setIsCmpOrdered(bool Val = true) { IsCmpOrdered = Val; }
+  bool getIsCmpOrdered() const { return IsCmpOrdered; }

Both calls to this function are explicit with the argument, so I think you can 
drop the default argument value.



Comment at: include/clang/Sema/Sema.h:4547
+  /// results are cached in ASTContext so they are accessible outside of Sema.
+  /// An error is emitted  if the types are not found or another error occurs.
+  ///

Extra whitespace between emitted and if.



Comment at: lib/AST/ComparisonCategories.cpp:1
+//===- ComparisonCategories.h - Three Way Comparison Data ---*- C++ 
-*-===//
+//

.cpp instead of .h



Comment at: lib/AST/ComparisonCategories.cpp:38
+ComparisonCategories::getInfoForType(QualType Ty) const {
+  auto OptKind = getCategoryForType(Ty);
+  assert(OptKind &&

Don't use `auto` here.



Comment at: lib/AST/ExprConstant.cpp:5084
+
+  BinOpEvaluatorBase(EvalInfo , ComparisonCategoryResult *CmpRes = 
nullptr)
+: ExprEvaluatorBaseTy(info), CmpResult(CmpRes) {

s/info/Info



Comment at: lib/AST/ExprConstant.cpp:6301
+  using CCR = ComparisonCategoryResult;
+  auto  = Info.Ctx.CompCategories.getInfoForType(E->getType());
+

Don't use `auto` here.



Comment at: lib/AST/ExprConstant.cpp:8589
+  bool IsSpaceship = E->getOpcode() == BO_Cmp;
+  auto  = this->Info;
   // We don't call noteFailure immediately because the assignment happens after

Don't use `auto` here. Also, perhaps the two variables can be sunk closer to 
where they're used?



Comment at: lib/AST/ExprConstant.cpp:8930
   if (LHSTy->isMemberPointerType()) {
-assert(E->isEqualityOp() && "unexpected member pointer operation");
+assert((E->isEqualityOp()) && "unexpected member pointer operation");
 assert(RHSTy->isMemberPointerType() && "invalid comparison");

Spurious parens?



Comment at: lib/AST/ExprConstant.cpp:10814
+// Check that all of the references to the result objects are ICE.
+auto  = Ctx.CompCategories.getInfoForType(Exp->getType());
+ICEDiag RetDiag(IK_ICE, E->getLocStart());

Don't use `auto` here.



Comment at: lib/AST/ExprConstant.cpp:10816
+ICEDiag RetDiag(IK_ICE, E->getLocStart());
+for (auto  : CmpInfo.Objects)
+  RetDiag = Worst(RetDiag, CheckICE(KV.second, Ctx));

`const auto &`?



Comment at: lib/CodeGen/CGExprAgg.cpp:892
+  QualType ArgTy =