[PATCH] D54472: Disable invalid isPodLike<> specialization

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

FWIW, I really like the direction you and Dave are pursuing. My random thoughts about how to make progress below:



================
Comment at: include/llvm/Support/type_traits.h:40-44
+#define LLVM_VERIFY_ISPODLIKE_CONSISTENCY(T)                                   \
+  static_assert(llvm::isPodLike<T>::value ==                                   \
+                    std::is_trivially_copyable<T>::value,                      \
+                "pod-like implies trivially copyable")
+
----------------
serge-sans-paille wrote:
> dblaikie wrote:
> > serge-sans-paille wrote:
> > > dblaikie wrote:
> > > > Does the presence of this macro mean "no one should ever specialize isPodLike"? & if they don't, then its certain that isPodLike == is_trivially_copyable, by definition?
> > > > 
> > > > If so, then I'm not sure of the importance of the macro - we should just say "don't specialize isPodLike, ever & leave it at that, maybe?
> > > Almost. It means « if you specialize ``isPodLike``, then make sure that for modern compiler, it does not conflict with std::is_trivially_copyable ».
> > > 
> > > Basically for modern compiler, we should have
> > > 
> > > ``template<class T> using isPodLike = std::is_trivially_copiable<T>;``
> > > 
> > > However, some compilers supported by LLVM don't implement that feature. In my opinion, it is still valid to speicalize ``isPodLike`` to ensure decent performance in these case. But not at the cost of UB.
> > OK, fair enough. 
> > 
> > Though does the performance matter on platforms that don't support the real trait? Which compilers/versions is that compared to the set of supported compilers/versions for building Clang/LLVM. (also, in part, the performance of Clang/LLVM is most important on a self-host build - correctness more broadly, but those interest in performance should likely be bootstrapping)
> > 
> > In any case, /if/ we're going to keep that performance feature (whereby LLVM developers can annotate types they know would pass is_trivially_copyable if it were available), probably validating its usage should be kept at the same place as the trait specializations, rather than in the usage?
> > 
> > Any chance of improving/changing Optional/SuccIterator to be trivially copyable when their parameters are trivially copyable?
> > 
> > (& is libc++'s std::pair trivially copyable when T1 and T2 are trivially copyable?)
> > probably validating its usage should be kept at the same place as the trait specializations, rather than in the usage?
> 
> The only solutipon I have for that is to define `struct isPodLike : details::isPodLike<T>`, perform the check in `isPodLike` and have user specialize `details::isPodLike` but nothing would prevent them from directly specializing `isPodLike` so I'm not a big fan of this soltion.
> 
> My best catch would be to define `isPodLike` as a template alias, that would make it non-speicalizable, no need for extra check and (slightly?) degraded performance for legacy compilers. I have the patch ready for that.
> 
> > Any chance of improving/changing Optional/SuccIterator to be trivially copyable when their parameters are trivially copyable?
> 
> Nop, they overload the assign operator, which need to be defaulted :-/
> In the case of Optionnal, I may give it a try, but probably in another commit.
> 
> Can't do anything for std::pair :-)
I think, long-term, we should just be checking for `std::is_trivially_copyable` as that's what we actually want. And we *should* be fixing the types to actually be trivially copyable as it will also make those types fast in standard library containers.

What I'm having trouble extracting from a quick read here is:

1) What types in LLVM do we want the `memcpy` optimization for which fail a modern compiler's `std::is_trivially_copyable`? We should fix those first IMO.

2) What compilers / standard libraries actually cannot implement `std::is_trivially_copyable`? I suspect we could simulate it very closely (without the specialization hacks) on most if not all of them.

I suspect we should move this to be an `llvm::is_trivially_copyable` that is implemented via simulation rather than specialization. I suspect we have enough compiler smarts available now, but maybe not. Even if we have to do some limited specialization, I would suggest renaming it to make the intent super clear.


https://reviews.llvm.org/D54472