[PATCH] D54472: Disable invalid isPodLike<> specialization

[serge via Phabricator via llvm-commits]

serge-sans-paille marked an inline comment as done.
serge-sans-paille added a comment.

@rsmith thanks a lot for the final review. I rebased the patch, did a final check with both GCC and Clang, and everything looks fine now. Waiting for a green light!



================
Comment at: docs/ProgrammersManual.rst:1459
 
-#. SmallVector understands ``isPodLike<Type>`` and uses realloc aggressively.
+#. SmallVector understands ``is_trivially_copyable<Type>`` and uses realloc aggressively.
 
----------------
Quuxplusone wrote:
> Bikeshed: This isn't technically an advantage over std::vector, since std::vector also does this. :)
> 
> I do think you should explicitly say `llvm::is_trivially_copyable<Type>` here, to distinguish it from `std::is_trivially_copyable<Type>` (which IIUC should not be used anywhere in the LLVM codebase). But maybe the `llvm::` qualification would be implicitly assumed by people who work with LLVM frequently?
I totally agreee on std::vector using the same technique. That's independent from that commit though :-)

And yeah, better be explicit about `llvm::is_trivially_copyable`.


================
Comment at: include/llvm/Analysis/BlockFrequencyInfoImpl.h:188
 
-    BlockNode() = default;
+    BlockNode() : Index(std::numeric_limits<uint32_t>::max()) {}
     BlockNode(IndexType Index) : Index(Index) {}
----------------
I still needed to rewrite it that way. Clang was happy with the original version bug gcc wasn't (i.e. it made `std::` and `llvm::` version behave differently).


================
Comment at: include/llvm/CodeGen/DIE.h:803
 protected:
-  ~DIEUnit() = default;
+  virtual ~DIEUnit() = default;
 
----------------
This class has a virtual member, so it needs a virtual destructor. And without that qualifier, `std::` and `llvm::` behave differently.


================
Comment at: include/llvm/Support/type_traits.h:163
+      (has_trivial_copy_constructor || has_trivial_copy_assign ||
+       has_trivial_move_constructor || has_trivial_move_assign);
+
----------------
rsmith wrote:
> serge-sans-paille wrote:
> > Quuxplusone wrote:
> > > Quuxplusone wrote:
> > > > serge-sans-paille wrote:
> > > > > Quuxplusone wrote:
> > > > > > serge-sans-paille wrote:
> > > > > > > serge-sans-paille wrote:
> > > > > > > > Quuxplusone wrote:
> > > > > > > > > This last clause — `(has_trivial_copy_constructor || has_trivial_copy_assign || has_trivial_move_constructor || has_trivial_move_assign)` — does not correspond to anything in the Standard. So:
> > > > > > > > > 
> > > > > > > > > https://godbolt.org/z/R5hCff
> > > > > > > > > ```
> > > > > > > > > struct S {
> > > > > > > > >     S(S&&) = delete;
> > > > > > > > > };
> > > > > > > > > static_assert(is_trivially_copyable<S>::value);
> > > > > > > > > ```
> > > > > > > > > ```
> > > > > > > > > error: static_assert failed "consistent behavior"
> > > > > > > > >   static_assert(value == std::is_trivially_copyable<T>::value, "consistent behavior");
> > > > > > > > >   ^             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > > > > ```
> > > > > > > > > I'm not sure but I would suggest just getting rid of this last clause.
> > > > > > > > From https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable (yeah that's not the standard, but I do hope it's derived from the standard)
> > > > > > > > 
> > > > > > > > > at least one copy constructor, move constructor, copy assignment operator, or move assignment operator is non-deleted
> > > > > > > > 
> > > > > > > > That's the intent I was trying to capture.
> > > > > > > > 
> > > > > > > > I'm okay with getting rid of this capture, especially when looking at https://godbolt.org/z/p3-Cy4.
> > > > > > > @Quuxplusone I had to reintroduce the ` (has_trivial_move_assign || has_trivial_move_constructor || has_trivial_copy_assign || has_trivial_copy_constructor)` condition, which is indeed a standard requirement. It basically states « if the class is not meant to be copied, don't allow to copy it ».
> > > > > > The standard trait "trivially copyable" is not related to copy-{constructibility,assignability}; it's its own thing.
> > > > > > 
> > > > > > I insist that if you're naming your thing "trivially copyable," it should exactly match the Standard trait's behavior, i.e., it should succeed at this test case:
> > > > > > 
> > > > > > ```
> > > > > > struct S {
> > > > > >     S(S&&) = delete;
> > > > > > };
> > > > > > static_assert(is_trivially_copyable<S>::value);
> > > > > > ```
> > > > > > 
> > > > > > Your current code doesn't pass that test. https://godbolt.org/z/j6pkaS
> > > > > > 
> > > > > > Any users of `is_trivially_copyable` should use it only //to enable perf optimizations//. They shouldn't be using it to ask, "//Should// I make a copy of this?" First of all, "make a copy" is underspecified. What they should have asked is "Should I copy-construct this?" or "Should I copy-assign this?" And we already have traits for that. Then, once they've already decided to make a copy (or maybe not to make any copies, but just to //move// the value without copying), //then// they should look at `is_trivially_copyable` to decide //how// to make that copy.
> > > > > I totally agree on your point. Unfortunately, I don't think it's possible to achieve a portable implementation of is_trivially_copyable without compiler support for all supported compiler version (including gcc 4.9). And that's exactly why this patch exist: to cope with this limitation, while ensuring the memcpy are valid, which is not the case with current implementation (which is both invalid for some type/compiler combination, and not conservative with respect to ABI because of the impact of `isPodLike` on ADT like `SmallVector`. So I would advocate for renaming this trait `is_memc(o)pyable` and ensuring that forall T instanciated in LLVM where `std::is_trivially_copyable` matters,  `is_memcopyable<T>::value == true` implies `std::is_trivially_copyable<T> == true`, which is currently the case (as we actually even verify that  `is_memcopyable<T>::value == std::is_trivially_copyable<T>::value`).
> > > > > 
> > > > > Does that sound OK to you?
> > > > I think that `is_memcpyable` sounds underdetermined: it ought to be `is_move_assignable_via_memcpy`, `is_copy_constructible_via_memcpy`, or whatever operation we're asserting is tantamount to memcpy. At which point you've just reinvented `is_trivially_move_assignable`, `is_trivially_copy_constructible` etc. (and here I'll insert a shameless plug for D50119 `is_trivially_relocatable`, because quite often the operation we want to replace is "relocating", not merely "copy-constructing" or "move-assigning").
> > > > 
> > > > The situation is complicated by the fact that right now the C++17 standard specifies that memcpying is technically permitted only for trivially copyable types. So if by `is_memcpyable` you mean "is memcpying technically permitted for this type under //any// circumstances — never mind whether it's semantically correct —" then `is_memcpyable` would be just a synonym for `std::is_trivially_copyable`. (But then specializing it would be UB.)
> > > > 
> > > > I think this patch has been evolving toward "Provide an `llvm::is_trivially_copyable` trait that is exactly the same as `std::is_trivially_copyable`, but portable." I think it should keep going in that direction.
> > > > 
> > > > If we say "Maybe the trait shouldn't be `is_trivially_copyable` at all, but something else," then that reopens the can of worms. (At that point, why not just keep the name `isPodLike`?)
> > > But on the other hand, I see that MSVC's standard library agrees with you that an immovable type is not `is_trivially_copyable`. (GCC/libstdc++, Clang/libc++, and ICC agree with me that it's unrelated.)
> > > https://godbolt.org/z/Gv_hcj
> > > So we'll never get `value == std::is_trivially_copyable<T>::value` to pass on all platforms.
> > > 
> > > So I won't block this review over the `(has_trivial_move_assign || has_trivial_move_constructor || has_trivial_copy_assign || has_trivial_copy_constructor)` condition. If you want it, you can keep it. But I hope that that line isn't necessary for correctness! I mean, I hope that if someone removes that line in the future, or switches from `llvm::is_trivially_copyable` to `std::is_trivially_copyable`, it won't cause silent breakage of any of LLVM's data structures. Is that reasonable?
> > > So I won't block this review
> > 
> > Cool.
> > 
> > > switches from llvm::is_trivially_copyable to std::is_trivially_copyable
> > In current state, thanks to the assert, `llvm::is_trivially_copyable` and `std::is_trivially_copyable` are equivalent *with respect to LLVM codebase*.
> The problem here is a bug in GCC and Clang's `__is_trivially_copyable`, not a bug in this patch. Example: https://godbolt.org/z/XKaLOa (The specific issue is that neither compiler implements DR1734 yet.)
> The standard trait "trivially copyable" is not related to copy-{constructibility,assignability}; it's its own thing.

Again, when recompiling with both GCC and Clang, I realized that they both agree with you; i double checked the clang source code and it also agrees with you, so I removed the final condition.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54472/new/

https://reviews.llvm.org/D54472