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

[Eric Fiselier via Phabricator via cfe-commits]

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


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




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


================
Comment at: lib/CodeGen/CGExprAgg.cpp:964
+    RHS = CGF.EmitAnyExpr(E->getRHS()).getAggregatePointer();
+    break;
+  case TEK_Complex:
----------------
rjmccall wrote:
> It looks like we don't actually support any aggregate types here, which I think is fine because comparing those types is only sensible for things like calls.  If you do want to pave the way for that, or (probably more usefully) for supporting complex types, you should make EmitCompare take the operands as RValues and just use EmitAnyExpr here without paying any attention to the evaluation kind.
Initially I thought the same thing, but apparently member pointers are Aggregates under the Microsoft ABI.

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


================
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:
> 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.
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?



================
Comment at: lib/Sema/SemaExpr.cpp:9955
+      // and direction polls
+      return buildResultTy(ComparisonCategoryType::StrongEquality);
+
----------------
rjmccall wrote:
> 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.
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). 


https://reviews.llvm.org/D45476