This is an archive of the discontinued LLVM Phabricator instance.

[C++2a] Implement operator<=> CodeGen and ExprConstant
ClosedPublic

Authored by EricWF on Apr 10 2018, 12:39 AM.

Details

Summary

This patch tackles long hanging fruit for the builtin operator<=> expressions. It is currently needs some cleanup before landing, but I want to get some initial feedback.

The main changes are:

  • Lookup, build, and store the required standard library types and expressions in ASTContext. By storing them in ASTContext we don't need to store (and duplicate) the required expressions in the BinaryOperator AST nodes.
  • Implement [expr.spaceship] checking, including diagnosing narrowing conversions.
  • Implement ExprConstant for builtin spaceship operators.
  • Implement builitin operator<=> support in CodeGenAgg. Initially I emitted the required comparisons using ScalarExprEmitter::VisitBinaryOperator, but this caused the operand expressions to be emitted once for every required cmp.
  • Implement [builtin.over] with modifications to support the intent of P0946R0. See the note on BuiltinOperatorOverloadBuilder::addThreeWayArithmeticOverloads for more information about the workaround.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
EricWF updated this revision to Diff 142326.Apr 12 2018, 9:22 PM

Address most of @rsmith's comments.

  • Back out changes to Expr.h.
  • Build comparison category types as needed, not eagerly.
  • Remove diagnostics about non-constexpr narrowing.

@rsmith I also tried to modify ExprConstant as requested, but I'm not sure I ended up with something better. But at least I'm no longer building a dummy BinaryOperator expression.
Let me know what you think.

EricWF updated this revision to Diff 142332.Apr 12 2018, 10:22 PM

Misc cleanup.

EricWF updated this revision to Diff 142336.Apr 12 2018, 11:35 PM

This revision further improves ExprConstant by splitting out evaluation of comparison operations, and report all of which using ComparisonCategoryResult.
I think this implementation will satisfy the concerns raised earlier.

EricWF updated this revision to Diff 142346.Apr 13 2018, 12:45 AM
  • Further improvements and cleanup to ExprConstant implementation.
rsmith added inline comments.Apr 13 2018, 1:39 AM
include/clang/AST/ComparisonCategories.h
78–79

Sorry, while editing this comment I managed to reverse it from what I meant. This should deal in NamedDecl* (or perhaps ValueDecl*) , not DeclRefExpr*.

include/clang/Basic/DiagnosticSemaKinds.td
9384–9385

void* is an object pointer type.

9393

Sounds good.

lib/Sema/SemaDeclCXX.cpp
8888

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.)

EricWF added inline comments.Apr 13 2018, 11:28 AM
include/clang/Basic/DiagnosticSemaKinds.td
9384–9385

Oh. right. 'object pointer' vs 'pointer-to-object'.

EricWF added inline comments.Apr 13 2018, 11:53 AM
lib/Sema/SemaDeclCXX.cpp
8888

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.

EricWF updated this revision to Diff 142453.Apr 13 2018, 12:04 PM

Remove diagnostics for void* composite types. I confused object pointer and pointer-to-object.

EricWF added inline comments.Apr 13 2018, 12:59 PM
include/clang/AST/ComparisonCategories.h
78–79

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 DeclRefExprs during evaluation just to throw them away, I think it makes sense to eagerly build the results as DeclRefExprs.

Does that make sense?

rsmith added inline comments.Apr 13 2018, 2:39 PM
include/clang/AST/ComparisonCategories.h
78–79

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.

lib/Sema/SemaDeclCXX.cpp
8888

You can't rely on Sema existing or having done this in the case where we're loading from an AST file.

EricWF updated this revision to Diff 142506.Apr 13 2018, 8:53 PM

Implement the lookup and building of comparison category types/values in ASTContext instead of Sema. When Sema checks a comparison category type, it performs full lookup, whereas ASTContext does not. Should Sema depend on ASTContext for performing lookup?

This change also moves ComparisonCategories.h from trafficking in DeclRefExprs, using VarDecl instead. For now ExprConstant and CGExprAgg build dummy DeclRefExprs for ease of evaluation. But I plan to fix this shortly, once I figure out how to do it.

EricWF updated this revision to Diff 142507.Apr 13 2018, 9:21 PM
  • No longer build dummy DeclRefExprs when evaluating constant expressions.
EricWF updated this revision to Diff 142508.Apr 13 2018, 9:42 PM
  • No longer build DeclRefExprs in CGExprAgg.cpp. This completes the transition from DeclRefExpr -> VarDecl.

@rsmith, Thanks for holding my hand and explaining the path forward to me. I appreciate your patience. :-D

EricWF marked 30 inline comments as done.Apr 13 2018, 9:55 PM
EricWF added inline comments.
include/clang/AST/ComparisonCategories.h
78–79

Ack. Done. I'm not sure why CodeGen gave me so much trouble. But you were right, it ended up being straight forward.

195

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

lib/Sema/SemaDeclCXX.cpp
8888

Ack. Thanks for sticking with me. I understand now.

Thanks! This is looking pretty close.

include/clang/AST/ComparisonCategories.h
79

Typo "Wether".

include/clang/Basic/DiagnosticSemaKinds.td
9377

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

9387

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.)

lib/AST/ComparisonCategories.cpp
52

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

lib/AST/ExprConstant.cpp
3784

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

8824

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?

lib/CodeGen/CGExprAgg.cpp
946–953

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

999–1000

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).

lib/Sema/SemaDeclCXX.cpp
8903–8916

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.

8936–8948

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

8966

I would somewhat prefer that we keep (say) a DenseSet of checked types on Sema rather than having Sema effectively own this flag that's held by ASTContext.

lib/Sema/SemaExpr.cpp
9727–9728

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).

9761–9776

Delete all of this? Once we get this far, we know it's a narrowing conversion, so should just be diagnosing it.

9794

This is not especially useful: C++2a is a moving target, so p3 might mean something else later. We usually include an excerpt describing what we're checking, if it's useful to do so.

9795

Redundant parens here.

9807–9810

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.

9814–9815

We use CK_IntegralCast for conversions between enumerations and integer types.

9816

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.

9887–9894

This is wrong for the three-way comparison case, where these conversions are only performed if one of the operands is of pointer type (see [expr.spaceship]p6). You could either delay decaying here, or check whether we performed a non-permitted decay after the fact.

9925–9927

Per http://wiki.edg.com/bin/view/Wg21jacksonville2018/P0946R0-Jax18, we should also end up here if either of the operands was (a null pointer constant) of non-pointer type prior to the conversions -- that is, in the pointer comparison cases where < would be invalid, we should return std::strong_equality.

9935–9936

You don't need this check; you already checked for function pointer types above, which are the only kind of non-object pointer.

9940–9941

Yes, you'll get here for at least Obj-C object pointers and block pointers.

10112

Three-way compare is not a relational comparison, so this change appears redundant? (Likewise in other places where you use this check.) Maybe we're passing the wrong value for IsRelational?

11942

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)?

rsmith added inline comments.Apr 14 2018, 7:57 AM
lib/Sema/SemaExpr.cpp
9928

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

EricWF marked 24 inline comments as done.Apr 14 2018, 4:13 PM
EricWF added inline comments.
lib/AST/ExprConstant.cpp
3784

Woops. It was, thanks!

lib/CodeGen/CGExprAgg.cpp
999–1000

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.

lib/Sema/SemaExpr.cpp
9727–9728

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.

9807–9810

I was thinking that if there wasn't already an issue for this behavior, there should be. Thanks for pointing it out.

11942

Ack. Removing the parameter and re-computing it from Opc.

EricWF updated this revision to Diff 142529.Apr 14 2018, 4:14 PM
EricWF marked 4 inline comments as done.
  • Address most inline comments, except for changes to SemaExpr. I'll follow up with those shortly.
EricWF marked 4 inline comments as done.Apr 14 2018, 4:43 PM
EricWF added inline comments.
lib/Sema/SemaExpr.cpp
9928

Is it OK if I go ahead and implement it anyway?

EricWF added inline comments.Apr 14 2018, 4:50 PM
lib/Sema/SemaExpr.cpp
9928

Nevermind. The changes seems involved enough they should be done separately.

EricWF marked 3 inline comments as done.Apr 14 2018, 5:15 PM
EricWF added inline comments.
lib/Sema/SemaExpr.cpp
9816

Do we still do usual arithmetic conversions if we have two enumerations of the same type?

EricWF updated this revision to Diff 142539.Apr 14 2018, 10:45 PM
EricWF marked 4 inline comments as done.
EricWF edited the summary of this revision. (Show Details)
  • Implement P0946R0. In particular reject comparisons of arrays, and allow comparisons of integrals with unscoped enumeration types.
  • Implement [built.over] for operator<=>, but with a twist. As currently specified the [over.built] doesn't play nice with P0946R0. Specifically it makes the following code ambiguous.

For example:

enum EnumA : int { A };
auto x = (A <=> (long)0); // ambiguous.

See the note on BuiltinOperatorOverloadBuilder::addThreeWayArithmeticOverloads for more information about the workaround.

EricWF updated this revision to Diff 142540.Apr 14 2018, 10:51 PM
  • Correct spelling and grammar in comments.
EricWF updated this revision to Diff 142592.Apr 15 2018, 10:12 PM

As suggested by @rsmith, Adjust addThreeWayArithmeticOverloads to add overloads for all possible combinations of promoted arithmetic types (ie. the same overloads added for relational operators).

rsmith added inline comments.Apr 17 2018, 3:16 AM
lib/Sema/SemaExpr.cpp
9794

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

9795

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.

9816

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.

9829–9832

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

9881–9890

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

9906

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

9907

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

EricWF updated this revision to Diff 142850.Apr 17 2018, 3:16 PM
EricWF marked 8 inline comments as done.

Address the newest batch of inline comments.

rjmccall added inline comments.Apr 23 2018, 9:05 PM
lib/CodeGen/CGExprAgg.cpp
1002

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.

EricWF added inline comments.May 3 2018, 1:58 PM
lib/CodeGen/CGExprAgg.cpp
1002

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.

lib/Sema/SemaExpr.cpp
9795

I'll remove the comment for now then.

9906

I'm bad at naming things. Is IsAnyPointerType better?

EricWF added inline comments.May 3 2018, 2:28 PM
lib/CodeGen/CGExprAgg.cpp
1002

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.

EricWF updated this revision to Diff 145137.May 3 2018, 7:42 PM

Address @rjmccall's comments, in particular fix codegen to handle non-trivially copyable types.

These most recent changes now cause CheckComparisonCategoryType to lookup and validate that we have an accessible and non-deleted copy constructor accepting a const lvalue.
The validity checks for the copy constructor aren't exhaustive, but should be sufficient for all but the most insane STL implementations.

EricWF updated this revision to Diff 145144.May 3 2018, 8:52 PM

Force declaration of implicit copy constructors when checking comparison category types. This allows lookups of implicitly declared copy constructors later via DeclContext.

rjmccall added inline comments.May 3 2018, 11:02 PM
lib/Sema/SemaDeclCXX.cpp
8931

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.

EricWF added inline comments.May 3 2018, 11:23 PM
lib/Sema/SemaDeclCXX.cpp
8931

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.

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 <reviews@reviews.llvm.org> wrote:

EricWF added inline comments.

Comment at: lib/Sema/SemaDeclCXX.cpp:8931
+ /*ConstArg*/ true, false, false, false, false);
+ auto *CopyCtor = cast_or_null<CXXConstructorDecl>(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

EricWF added a comment.EditedMay 4 2018, 1:31 AM

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 <compare>
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 <foo.h> // 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 <compare>, but it
  // does generate code for the inline function, which requires <compare>;
  // 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.

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, and must be of a very specific form. Also note that it's
always the case that at least one copy constructor participates in 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).

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).

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.

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
attempting to implement what you're suggesting, and work past the
problematic example given earlier.

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.

Ah, I see. So the standard already requires the category types to be literal.
So we can always avoid ODR use.

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.

The variables are declared as inline and constexpr. There won't be binary compatibility
issues nor problems with opaque definitions.

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

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 <compare>
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 <foo.h> // 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 <compare>, but it
  // does generate code for the inline function, which requires <compare>;
  // 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
attempting to implement what you're suggesting, and work past the
problematic example given earlier.

Okay, thanks.

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.

Ah, I see. So the standard already requires the category types to be literal.
So we can always avoid ODR use.

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.

The variables are declared as inline and constexpr. There won't be binary compatibility
issues nor problems with opaque definitions.

Ah, right, I'd forgotten that C++ finally added inline variables. Yes, that would solve
the opaqueness and compatibility problems.

John.

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

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

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.

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.

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

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

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.

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.

EricWF updated this revision to Diff 145315.May 4 2018, 4:02 PM

Revert changes for non-trivial constructors. After speaking to Richard he agrees that it's OK to expect the comparison category types to be trivially copyable.
We now diagnose STL implementations which do not meet this requirement.

rsmith added inline comments.May 4 2018, 4:31 PM
lib/AST/ExprConstant.cpp
8853

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

lib/CodeGen/CGExprAgg.cpp
971

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.)

EricWF marked an inline comment as done.May 4 2018, 4:43 PM
EricWF added inline comments.
lib/CodeGen/CGExprAgg.cpp
971

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?

rjmccall added inline comments.May 4 2018, 5:38 PM
include/clang/AST/ASTContext.h
1986

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

include/clang/AST/ComparisonCategories.h
72

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.)

207

Please add a comment explaining what the keys are.

lib/AST/ComparisonCategories.cpp
86

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.

lib/CodeGen/CGExprAgg.cpp
971

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.

lib/Sema/SemaDeclCXX.cpp
8929

Alright, works for me.

8956

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.

lib/Sema/SemaExpr.cpp
9816

isEnumeralType is just checking for an any enum type, but I assume we can't use a spaceship to compare a scoped enum type, right? Since the ordinary relational operators aren't allowed either.

9825

I believe this will assert if the underlying type of the enum is bool.

9963

Should we generate a tautological warning about comparisons on nullptr_t that aren't the result of some kind of macro/template expansion?

EricWF marked 18 inline comments as done.May 4 2018, 9:41 PM
EricWF added inline comments.
include/clang/AST/ASTContext.h
1986

Woops. No, it is not accurate. It's a remnant of an earlier attempt. I'll remove it.

include/clang/AST/ComparisonCategories.h
72

Ack.

Do you want std::array or something slightly more conservative like llvm::SmallVector<T, 4>?

lib/AST/ComparisonCategories.cpp
86

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.

lib/CodeGen/CGExprAgg.cpp
971

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 Sema that validate that the caches have been populated correctly, and that the required constraints hold on the comparison category type and it's instances.

When we import a module, the Sema checking isn't repeated, but if anything from that loaded module requires the comparison category caches, then they must have been well-formed when we initially checked them, and so they should also be well formed when we lazely populate them.

1002

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

Yes. Sema now emits a diagnostic for non-trivially copyable STL implementations.

lib/Sema/SemaDeclCXX.cpp
8956

I don't think that would be correct. For example, the following code should only issue one diagnostic.

auto foo(int x, int y) { return x <=> y; } // expected-error {{include <compare>}}
#include <compare>
auto bar(int x, int y) { return x <=> y; } // OK

Also, like with <initializer_list> we probably want the following code to emit two diagnostics:

void foo(int x, int y) {
  (void)(x <=> y); // expected-error
  (void)(x <=> y); // expected-error
}

When <compare> is ill-formed, I believe the correct behavior is to emit a single diagnostic for each expression
which requires the header. Otherwise, we could end up with a ton of ill-formed but undiagnosed code.

lib/Sema/SemaExpr.cpp
9816

You can compare two enums of the same type (scoped or unscoped), or an unscoped enum and an arithmetic type.

(Also, ordinary relational operators are supported for scoped enum types).

9825

Ack. It does indeed.

What's the correct way to promote the boolean to an integer?

EricWF marked 12 inline comments as done.May 4 2018, 9:59 PM
EricWF added inline comments.
lib/Sema/SemaExpr.cpp
9825

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?

9963

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?

EricWF updated this revision to Diff 145350.May 4 2018, 10:02 PM
EricWF marked an inline comment as done.

Address most inline comments.

  • Check and diagnose failed assumptions about the STL in Sema.
  • Change CGExprAgg to select between the integer values, and not the addresses of the variables.
  • Fix assertion when converting enums with the underlying type of bool
  • Convert map/set data structures to arrays when possible (this fixes the bug with non-stable keys).
rjmccall added inline comments.May 4 2018, 10:24 PM
include/clang/AST/ComparisonCategories.h
72

std::array is definitely better here.

lib/AST/ComparisonCategories.cpp
86

Sounds good.

lib/CodeGen/CGExprAgg.cpp
971

Right. I do actually believe the clever argument in normal situations. :) It just seems brittle enough that I don't really trust that there won't be some corner case that bypasses it, or some future change to modules that breaks it in ways we fail to anticipate.

lib/Sema/SemaDeclCXX.cpp
8956

I think there are ways to handle the subsequent-inclusion thing, but I'm not too attached to the idea of not emitting redundant errors here.

lib/Sema/SemaExpr.cpp
9816

Huh. How did I mess that up? Okay, makes sense to me.

9825

A CK_IntegralToBoolean would also be reasonable, I think. Or thinking about, it also wouldn't be unreasonable to just weaken the assertion when the integral type is an enum of bool underlying type.

9963

I thought we had some warnings in that space, but maybe not that one. Don't worry about it.

rjmccall added inline comments.May 4 2018, 10:56 PM
lib/CodeGen/CGExprAgg.cpp
964

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.

1000

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

EricWF marked 8 inline comments as done.May 5 2018, 12:39 AM
EricWF added inline comments.
include/clang/AST/ComparisonCategories.h
72

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.

lib/CodeGen/CGExprAgg.cpp
964

Initially I thought the same thing, but apparently member pointers are Aggregates under the Microsoft ABI.

I'll give trafficking in RValues, but all the functions EmitCompare calls use Value*, so it'll take some work.

971

My inclination is to agree with you. As a Clang newbie I would much rather delegate to other functions than re-invent them partially.

However, I'm not sure what you mean by "normal situation". As I'll argue below, the non-normal situation of interacting with the STL is actually better than the "normal situation", if by "normal situation" we mean "dealing with normal, user-provided, code".

My position as an STL maintainer is different. The compiler and STL have a special relationship. I'm sure I could find a dozen ways to break any of <initializer_list>, <new>, <coroutine>, <typeinfo>, exception_ptr, ect. That is, most interfaces between the library and language are fragile (by design?). If, as an STL maintainer, my implementation didn't work with a given compiler, i would likely fix the library long before I complained about the compiler (But that's just my experience).

One upside of these STL/compiler interfaces is that they're so widely used that any bug we do introduce will be discovered long before we release the compiler.

lib/Sema/SemaExpr.cpp
9825

Hmm. isn't CK_IntegralToBoolean for casting Integral/Enum -> Bool, and not visa versa?

I'm not sure i fully understand the assertion enough. In particular, why operands which reference enumerators with a non-boolean underlying type don't hit it. It seems expressions of the non-boolean case are rvalues, but in the boolean case they're lvalues?

9963

We do have warnings in that space. They're in diagnoseTautologicalComparison, and they do warn when the LHS and RHS name the same entity, but not necessarily the same value (as we have when comparing two nullptrs).

It seems reasonable to also warn when comparing two instances of a mono-state type. And I would be happy to add it (and probably will try, but after this patch lands).

EricWF updated this revision to Diff 145355.May 5 2018, 1:32 AM
EricWF marked 2 inline comments as done.
  • Adjust how a few of the caches represent themselves.
  • Misc cleanup.
EricWF added inline comments.May 5 2018, 1:34 AM
lib/CodeGen/CGExprAgg.cpp
964

*I'll give trafficking in RValues a shot, but ...*

EricWF updated this revision to Diff 145356.May 5 2018, 2:05 AM

Misc cleanup.

EricWF marked 7 inline comments as done.May 5 2018, 2:11 AM
EricWF updated this revision to Diff 145362.May 5 2018, 3:56 AM

Fix a bug where we fail to update the caches when we see a forward declaration of any of the STL types before we see the full definition.

EricWF updated this revision to Diff 145364.May 5 2018, 4:05 AM

Remove accidentally debug print statements.

EricWF added inline comments.May 5 2018, 4:21 AM
lib/AST/ExprConstant.cpp
8853

@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?

EricWF updated this revision to Diff 145387.May 5 2018, 3:05 PM

More misc cleanup.

EricWF updated this revision to Diff 145389.May 5 2018, 4:48 PM
EricWF marked 2 inline comments as done.

Implement support for complex types. If the element types is floating point the comparisons produce std::weak_equality and std::strong_equality otherwise.

rsmith added inline comments.May 5 2018, 5:39 PM
lib/AST/ExprConstant.cpp
8853

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.

rjmccall added inline comments.May 7 2018, 11:03 AM
lib/CodeGen/CGExprAgg.cpp
924

Dead code?

964

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.

EricWF updated this revision to Diff 145505.May 7 2018, 11:25 AM
EricWF marked 4 inline comments as done.

Attempt to clean code gen for operands as requested by @rjmccall .

lib/AST/ExprConstant.cpp
8853

Ah, I didn't think about how errors were handled. Thank you.

lib/CodeGen/CGExprAgg.cpp
924

Woops. Removed.

964

OK, I think I've cleaned it up. Let me know what you think.

EricWF marked 6 inline comments as done.May 7 2018, 11:27 AM
rjmccall accepted this revision.May 7 2018, 12:49 PM

Fine, that works.

This revision is now accepted and ready to land.May 7 2018, 12:49 PM
EricWF marked 2 inline comments as done.May 7 2018, 1:05 PM
EricWF edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.
rsmith added inline comments.May 7 2018, 2:13 PM
include/clang/AST/ASTContext.h
1983

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

include/clang/AST/ComparisonCategories.h
57

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

85–104

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

lib/AST/ComparisonCategories.cpp
26

classes -> class's

44–47

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

lib/CodeGen/CGExprAgg.cpp
959

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

lib/StaticAnalyzer/Core/ExprEngine.cpp
2899–2937

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

test/SemaCXX/compare-cxx2a.cpp
40

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

test/SemaCXX/std-compare-cxx2a.cpp
7

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.