Page MenuHomePhabricator

[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

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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
8829 ↗(On Diff #145315)

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

lib/CodeGen/CGExprAgg.cpp
971 ↗(On Diff #145315)

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 ↗(On Diff #145315)

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 ↗(On Diff #145315)

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
71 ↗(On Diff #145315)

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

206 ↗(On Diff #145315)

Please add a comment explaining what the keys are.

lib/AST/ComparisonCategories.cpp
85 ↗(On Diff #145315)

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 ↗(On Diff #145315)

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 ↗(On Diff #145315)

Alright, works for me.

8956 ↗(On Diff #145315)

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 ↗(On Diff #142508)

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 ↗(On Diff #145315)

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

9955 ↗(On Diff #145315)

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 ↗(On Diff #145315)

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

include/clang/AST/ComparisonCategories.h
71 ↗(On Diff #145315)

Ack.

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

lib/AST/ComparisonCategories.cpp
85 ↗(On Diff #145315)

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
1002 ↗(On Diff #142850)

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.

971 ↗(On Diff #145315)

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.

lib/Sema/SemaDeclCXX.cpp
8956 ↗(On Diff #145315)

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 ↗(On Diff #142508)

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 ↗(On Diff #145315)

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 ↗(On Diff #145315)

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?

9955 ↗(On Diff #145315)

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
71 ↗(On Diff #145315)

std::array is definitely better here.

lib/AST/ComparisonCategories.cpp
85 ↗(On Diff #145315)

Sounds good.

lib/CodeGen/CGExprAgg.cpp
971 ↗(On Diff #145315)

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 ↗(On Diff #145315)

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 ↗(On Diff #142508)

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

9825 ↗(On Diff #145315)

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.

9955 ↗(On Diff #145315)

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 ↗(On Diff #145350)

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 ↗(On Diff #145350)

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
71 ↗(On Diff #145315)

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
971 ↗(On Diff #145315)

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.

964 ↗(On Diff #145350)

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.

lib/Sema/SemaExpr.cpp
9825 ↗(On Diff #145315)

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?

9955 ↗(On Diff #145315)

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 ↗(On Diff #145350)

*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
8829 ↗(On Diff #145315)

@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
8829 ↗(On Diff #145315)

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 ↗(On Diff #145389)

Dead code?

964 ↗(On Diff #145350)

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
8829 ↗(On Diff #145315)

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

lib/CodeGen/CGExprAgg.cpp
924 ↗(On Diff #145389)

Woops. Removed.

964 ↗(On Diff #145350)

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 ↗(On Diff #145505)

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
56 ↗(On Diff #145505)

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

84–103 ↗(On Diff #145505)

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
25 ↗(On Diff #145505)

classes -> class's

43–46 ↗(On Diff #145505)

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

lib/CodeGen/CGExprAgg.cpp
959 ↗(On Diff #145505)

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 ↗(On Diff #145505)

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

test/SemaCXX/compare-cxx2a.cpp
40 ↗(On Diff #145505)

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

test/SemaCXX/std-compare-cxx2a.cpp
6 ↗(On Diff #145505)

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.