This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by krzysz00 on Jan 16 2023, 10:41 AM.

Details

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.

Diff Detail

Event Timeline

krzysz00 created this revision.Jan 16 2023, 10:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 10:41 AM
krzysz00 requested review of this revision.Jan 16 2023, 10:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2023, 10:41 AM

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 updated this revision to Diff 490173.Jan 18 2023, 8:24 AM

Address review comments

krzysz00 updated this revision to Diff 490226.Jan 18 2023, 10:33 AM

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

This revision was not accepted when it landed; it landed in state Needs Review.Jan 18 2023, 11:56 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
sameerds reopened this revision.Jan 18 2023, 5:31 PM

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.

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)

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.

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?

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.

sameerds accepted this revision.Jan 19 2023, 5:03 PM
This revision is now accepted and ready to land.Jan 19 2023, 5:03 PM
krzysz00 closed this revision.Jan 23 2023, 8:08 AM