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

[John McCall via Phabricator via cfe-commits]

rjmccall added inline comments.


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


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


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


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


================
Comment at: lib/Sema/SemaExpr.cpp:9816
+    RHS = S.ImpCastExprToType(RHS.get(), Type, CK_BitCast);
+  } else {
+    // C++2a [expr.spaceship]p4
----------------
EricWF wrote:
> 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).
Huh.  How did I mess that up?  Okay, makes sense to me.


================
Comment at: lib/Sema/SemaExpr.cpp:9825
+    LHS = S.ImpCastExprToType(LHS.get(), IntType, CK_IntegralCast);
+    RHS = S.ImpCastExprToType(RHS.get(), IntType, CK_IntegralCast);
+    LHSType = RHSType = IntType;
----------------
EricWF wrote:
> EricWF wrote:
> > rjmccall wrote:
> > > I believe this will assert if the underlying type of the enum is `bool`.
> > Ack. It does indeed.
> > 
> > What's the correct way to promote the boolean to an integer?
> I seem to have solved the problem for enums with an underlying type of bool by first performing LValueToRValue conversions, followed by a conversion to `int` instead of `bool`.
> 
> Does that sound reasonable?
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.


================
Comment at: lib/Sema/SemaExpr.cpp:9955
+      // and direction polls
+      return buildResultTy(ComparisonCategoryType::StrongEquality);
+
----------------
EricWF wrote:
> rjmccall wrote:
> > Should we generate a tautological warning about comparisons on `nullptr_t` that aren't the result of some kind of macro/template expansion?
> Probably? But we don't currently do it for equality operators, so perhaps this could be done in a follow up patch which adds the diagnostic for both equality and three-way comparisons?
I thought we had some warnings in that space, but maybe not that one.  Don't worry about it.


https://reviews.llvm.org/D45476