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

[Eric Fiselier via Phabricator via cfe-commits]

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


================
Comment at: include/clang/AST/ASTContext.h:1986
+  /// This object needs to be initialized by Sema the first time it checks
+  /// a three-way comparison.
+  ComparisonCategories CompCategories;
----------------
rjmccall wrote:
> Is this comment accurate?  Because if your test case with a deserialized comparison operator is supposed to work, we certainly don't get that.
Woops. No, it is not accurate. It's a remnant of an earlier attempt. I'll remove it.


================
Comment at: include/clang/AST/ComparisonCategories.h:71
+  /// standard library. The key is a value of ComparisonCategoryResult.
+  mutable llvm::DenseMap<char, VarDecl *> Objects;
+
----------------
rjmccall wrote:
> We expect this map to have at least two of the seven result types, and probably three or four, right?  It should probably just be an array; it'll both be faster and use less memory.
> 
> (The similar map in `ComparisonCategories` is different because we expect it to be empty in most translation units.)
Ack.

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


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

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


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





================
Comment at: lib/CodeGen/CGExprAgg.cpp:1002
+  return EmitFinalDestCopy(
+      E->getType(), CGF.MakeNaturalAlignAddrLValue(Select, E->getType()));
+}
----------------
EricWF wrote:
> EricWF wrote:
> > rjmccall wrote:
> > > Is there something in Sema actually validating that the comparison types is trivially copyable?  Because EmitFinalDestCopy does not work on non-trivial types.
> > > 
> > > Also, are we certain that we're allowed to do a copy from an actual global variable here instead of potentially a constexpr evaluation of the variable reference?  And if we are doing a copy, are we registering ODR-uses of all the variables in Sema?
> > > 
> > > I don't think you should worry about trying to generate a select between the "actual" comparison result values.  At least not yet.
> > There is nothing is Sema validating that these comparison types are trivially copyable, and according to the standard they don't need to be.
> > I assumed we only ended up in `CGExprAgg` if the types were trivially copyable. But I'll look into implementing this for non-trivially copyable comparison types (although we'll probably never actually encounter them).
> > 
> > > Also, are we certain that we're allowed to do a copy from an actual global variable here instead of potentially a constexpr evaluation of the variable reference?
> > 
> > I'm not sure exactly what this means. How would I observe the difference?
> > 
> > >And if we are doing a copy, are we registering ODR-uses of all the variables in Sema?
> > 
> > Probably not. I'll double check that.
> > 
> > > I don't think you should worry about trying to generate a select between the "actual" comparison result values. At least not yet.
> > 
> > I'm not sure exactly what you mean by this.
> To follow up:
> 
> >> And if we are doing a copy, are we registering ODR-uses of all the variables in Sema?
> >
> > Probably not. I'll double check that.
> 
> We do mark the potential result variables as ODR-used when we first check them when building builtin and defaulted comparison operators.
> 
> >> I don't think you should worry about trying to generate a select between the "actual" comparison result values. At least not yet.
> >
> > I'm not sure exactly what you mean by this.
> 
> Sorry, I see what you mean. You're talking about the comment. Richard asked me to leave that TODO there.
> 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.


================
Comment at: lib/Sema/SemaDeclCXX.cpp:8956
+  // the cache and return the newly cached value.
+  FullyCheckedComparisonCategories.insert(Info);
+  return Info->getType();
----------------
rjmccall wrote:
> 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.
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.



================
Comment at: lib/Sema/SemaExpr.cpp:9816
+    RHS = S.ImpCastExprToType(RHS.get(), Type, CK_BitCast);
+  } else {
+    // C++2a [expr.spaceship]p4
----------------
rjmccall wrote:
> rsmith wrote:
> > EricWF wrote:
> > > rsmith wrote:
> > > > We still need to apply the usual arithmetic conversions after converting enumerations to their underlying types (eg, `<=>` on `enum E : char` converts the operands first to `char` then to `int`). You could remove the `else` here and make this stuff unconditional, but it's probably better to sidestep the extra work and convert directly to the promoted type of the enum's underlying type.
> > > Do we still do usual arithmetic conversions if we have two enumerations of the same type?
> > Formally, yes: "If both operands have the same enumeration type E, the operator yields the result of converting the operands to the underlying type of E and applying <=> to the converted operands."
> > 
> > The recursive application of `<=>` to the converted operands will perform the usual arithmetic conversions.
> `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.
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).


================
Comment at: lib/Sema/SemaExpr.cpp:9825
+    LHS = S.ImpCastExprToType(LHS.get(), IntType, CK_IntegralCast);
+    RHS = S.ImpCastExprToType(RHS.get(), IntType, CK_IntegralCast);
+    LHSType = RHSType = IntType;
----------------
rjmccall wrote:
> I believe this will assert if the underlying type of the enum is `bool`.
Ack. It does indeed.

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


https://reviews.llvm.org/D45476