[llvm][GenericUniformity] Hack around strict is_invocable() checks

[krzysz00]

Summary

With recent (> 15, as far as I can tell, possibly > 16) clang, c++17,
and GNU's libstdc++ (versions 9 and 10 and maybe others), LLVM fails
to compile due to an is_invocable() check in unique_ptr::reset().

To resolve this issue, add a template argument to ImplDeleter to make
things work.

[sameerds]

Thanks for figuring this out! I just have a problem with the template parameter name "DelT", which should be easy to fix.

llvm/include/llvm/ADT/GenericUniformityInfo.h

70

Is the default type required? Also, "DelT" makes it sound like it's the type of the deleter rather. Since this struct is the "Impl" Deleter, either name it "ImplT" or "DataT" or something like that. I would recommend "ImplT" ... it can't conflict within its own scope.

[krzysz00]

Address review comments

[krzysz00]

Pull the deleter out of nesting in the hopes that thas + some forward declarations will fix the linker errors

[sameerds]

I see that the change has landed without review, but I am actually not okay with the new name. The ImplDeleter is a detail internal to the GenerericAnalysisInfo, and it's a bit unclean to pull it out as a top-level name. I would prefer putting it back inside.

[krzysz00]

Unfortunately, moving the deleter inside by this diff

diff --git a/llvm/include/llvm/ADT/GenericUniformityImpl.h b/llvm/include/llvm/ADT/GenericUniformityImpl.h
index 06d9b417ebde..f0f1f8e79c8e 100644
--- a/llvm/include/llvm/ADT/GenericUniformityImpl.h
+++ b/llvm/include/llvm/ADT/GenericUniformityImpl.h
@@ -467,8 +467,9 @@ private:
                            ConstValueRefT Val) const;
 };
 
+template <typename ContextT>
 template <typename ImplT>
-void GenericUniformityAnalysisImplDeleter<ImplT>::operator()(ImplT *Impl) {
+void GenericUniformityInfo<ContextT>::ImplDeleter<ImplT>::operator()(ImplT *Impl) {
   delete Impl;
 }
 
diff --git a/llvm/include/llvm/ADT/GenericUniformityInfo.h b/llvm/include/llvm/ADT/GenericUniformityInfo.h
index 24807bdc1c35..86d3d82f807a 100644
--- a/llvm/include/llvm/ADT/GenericUniformityInfo.h
+++ b/llvm/include/llvm/ADT/GenericUniformityInfo.h
@@ -24,14 +24,6 @@ namespace llvm {
 class TargetTransformInfo;
 
 template <typename ContextT> class GenericUniformityAnalysisImpl;
-template <typename ImplT> struct GenericUniformityAnalysisImplDeleter {
-  // Ugly hack around the fact that recent (> 15.0) clang will run into an
-  // is_invocable() check in some GNU libc++'s unique_ptr implementation
-  // and reject this deleter if you just make it callable with an ImplT *,
-  // whether or not the type of ImplT is spelled out.
-  using pointer = ImplT *;
-  void operator()(ImplT *Impl);
-};
 
 template <typename ContextT> class GenericUniformityInfo {
 public:
@@ -72,8 +64,17 @@ public:
 private:
   using ImplT = GenericUniformityAnalysisImpl<ContextT>;
 
+  template <typename ImplT> struct ImplDeleter {
+    // Ugly hack around the fact that recent (> 15.0) clang will run into an
+    // is_invocable() check in some GNU libc++'s unique_ptr implementation
+    // and reject this deleter if you just make it callable with an ImplT *,
+    // whether or not the type of ImplT is spelled out.
+    using pointer = ImplT *;
+    void operator()(ImplT *Impl);
+  };
+
   FunctionT *F;
-  std::unique_ptr<ImplT, GenericUniformityAnalysisImplDeleter<ImplT>> DA;
+  std::unique_ptr<ImplT, ImplDeleter<ImplT>> DA;
 
   GenericUniformityInfo(const GenericUniformityInfo &) = delete;
   GenericUniformityInfo &operator=(const GenericUniformityInfo &) = delete;
diff --git a/llvm/lib/Analysis/UniformityAnalysis.cpp b/llvm/lib/Analysis/UniformityAnalysis.cpp
index 8ed5af8a8d1c..2b2cb3648d62 100644
--- a/llvm/lib/Analysis/UniformityAnalysis.cpp
+++ b/llvm/lib/Analysis/UniformityAnalysis.cpp
@@ -87,8 +87,8 @@ bool llvm::GenericUniformityAnalysisImpl<SSAContext>::usesValueFromCycle(
 // This ensures explicit instantiation of
 // GenericUniformityAnalysisImpl::ImplDeleter::operator()
 template class llvm::GenericUniformityInfo<SSAContext>;
-template struct llvm::GenericUniformityAnalysisImplDeleter<
-    llvm::GenericUniformityAnalysisImpl<SSAContext>>;
+template struct llvm::GenericUniformityInfo<SSAContext>::ImplDeleter<
+  llvm::GenericUniformityAnalysisImpl<SSAContext>>;
 
 //===----------------------------------------------------------------------===//
 //  UniformityInfoAnalysis and related pass implementations
diff --git a/llvm/lib/CodeGen/MachineUniformityAnalysis.cpp b/llvm/lib/CodeGen/MachineUniformityAnalysis.cpp
index 2fe5e40a58c2..d6d0cd48af8b 100644
--- a/llvm/lib/CodeGen/MachineUniformityAnalysis.cpp
+++ b/llvm/lib/CodeGen/MachineUniformityAnalysis.cpp
@@ -113,7 +113,7 @@ bool llvm::GenericUniformityAnalysisImpl<MachineSSAContext>::usesValueFromCycle(
 // This ensures explicit instantiation of
 // GenericUniformityAnalysisImpl::ImplDeleter::operator()
 template class llvm::GenericUniformityInfo<MachineSSAContext>;
-template struct llvm::GenericUniformityAnalysisImplDeleter<
+template struct llvm::GenericUniformityInfo<MachineSSAContext>::ImplDeleter<
     llvm::GenericUniformityAnalysisImpl<MachineSSAContext>>;
 
 MachineUniformityInfo

makes the build error come back.

So, even if it's ugly, it fixes the build.

And working build trumps nice names (and I needed the build un-broken, so I wasn't going to sit around and wait for more review)

[sameerds]

I don't understand. There three versions in this review, and the first two did not involve moving the deleter type out. Were those versions not working as expected? And no, I think "working tree" is a rather weak excuse for pushing a patch when there is active involvement in the review, especially when things are clearly not broken in upstream.

[sameerds]

In D141865#4062909, @krzysz00 wrote:

Pull the deleter out of nesting in the hopes that thas + some forward declarations will fix the linker errors

"hope" does not inspire much confidence here. And also what linker error? I thought the original problem was a frontend error?

[krzysz00]

The second version of the diff, which was one that made the build error (with what I'd consider a supported set of compilers) go away, caused linker errors in the pre-merge checks because the ImplDeleter template wasn't instantiated. Adding instantiations caused the build error to return, and the fix for that was to move the deleter out of the GenericUniformityInfo into its own struct.

I think part of the trouble here was that GenericUniformityInfo<> is an incomplete type at the points where the reset() call is being instantiated, and so the is_invocable fails.

llvm/include/llvm/ADT/GenericUniformityImpl.h

private:
   bool usesValueFromCycle(const InstructionT &I, const CycleT &DefCycle) const;
 
   /// \brief Whether \p Val is divergent when read in \p ObservingBlock.
   bool isTemporalDivergent(const BlockT &ObservingBlock,
                            ConstValueRefT Val) const;
 };
 
 template <typename ContextT>
-void GenericUniformityInfo<ContextT>::ImplDeleter::operator()(
-    GenericUniformityAnalysisImpl<ContextT> *Impl) {
+template <typename ImplT>
+void GenericUniformityInfo<ContextT>::ImplDeleter<ImplT>::operator()(
+    ImplT *Impl) {
   delete Impl;
 }
 
 /// Compute divergence starting with a divergent branch.
 template <typename ContextT> class DivergencePropagator {
 public:
   using BlockT = typename ContextT::BlockT;
   using DominatorTreeT = typename ContextT::DominatorTreeT;

llvm/include/llvm/ADT/GenericUniformityInfo.h

public:
   bool isUniform(ConstValueRefT V) const { return !isDivergent(V); }
 
   bool hasDivergentTerminator(const BlockT &B);
 
   void print(raw_ostream &Out) const;
 
 private:
   using ImplT = GenericUniformityAnalysisImpl<ContextT>;
-  struct ImplDeleter {
-    void operator()(GenericUniformityAnalysisImpl<ContextT> *Impl);
+  // Ugly hack around the fact that recent (> 15.0) clang will run into an
+  // is_invocable() check in some GNU libc++'s unique_ptr implementation
+  // and reject this deleter if you just make it callable with an ImplT *,
+  // whether or not the type of ImplT is spelled out.
+  template <typename ImplT> struct ImplDeleter {
+    void operator()(ImplT *Impl);
   };
 
   FunctionT *F;
-  std::unique_ptr<ImplT, ImplDeleter> DA;
+  std::unique_ptr<ImplT, ImplDeleter<ImplT>> DA;
 
   GenericUniformityInfo(const GenericUniformityInfo &) = delete;
   GenericUniformityInfo &operator=(const GenericUniformityInfo &) = delete;
 };
 
 } // namespace llvm
 
 #endif // LLVM_ADT_GENERICUNIFORMITYINFO_H