[Verifier] Verify all blockaddress constants left over have at least one user.

[fhahn]

Summary

Some passes currently do not properly clean up blockaddress constants,
which limits further optimizations, as the blocks remain used by the
constant and remain marked as having their address taken.

With this patch and D64936, the following tests are failing:

LLVM :: Transforms/ConstProp/basictest.ll
LLVM :: Transforms/IndirectBrExpand/basic.ll

[brzycki]

Hello @fhahn , it looks like more tests are failing on tip 7df225dfc25adc8371188dc57f3adf300b0bd697 when I tested on x86_64:

Failing Tests (4):
    LLVM :: CodeGen/X86/retpoline.ll
    LLVM :: CodeGen/X86/speculative-load-hardening-indirect.ll
    LLVM :: Transforms/ConstProp/basictest.ll
    LLVM :: Transforms/IndirectBrExpand/basic.ll

llvm/lib/IR/Verifier.cpp

4985

Is it possible to add the name of the block/function to this assert? When looking at basic.ll I wasn't sure if the block in question was bb or entry of @test2().

[efriedma]

I'm not sure this is something we can reasonably enforce without making big changes in other areas of the compiler. For example, we call eraseFromParent() all over the place, and some large fraction of those calls could eliminate the last use of a blockaddress.

Places that need this soft of cleanup for global variables usually call Constant::removeDeadConstantUsers(). Not sure if we try to do anything like that for blockaddress constants.

[fhahn]

In D64984#1598258, @efriedma wrote:

I'm not sure this is something we can reasonably enforce without making big changes in other areas of the compiler. For example, we call eraseFromParent() all over the place, and some large fraction of those calls could eliminate the last use of a blockaddress.

Places that need this soft of cleanup for global variables usually call Constant::removeDeadConstantUsers(). Not sure if we try to do anything like that for blockaddress constants.

Yep it is probably not realistic to get everything clean soon given that it also isn't a huge issue. This might surface some obvious cases/places to clean up though :)

[fhahn]

Turns out clang can also generate unused block address constants, e.g. for

static a(void) {
b:
  &&b;
}
c() { a(); }

Maybe it would still make sense to add it disabled-by-default?

[fhahn]

I am not sure if this is worth pursuing further, given that clang itself may introduce unused block address constants.

llvm/lib/IR/Verifier.cpp

 //  * A landingpad instruction must be the first non-PHI instruction in the
 //    block.
 //  * Landingpad instructions must be in a function with a personality function.
 //  * All other things that are tested by asserts spread about the code...
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/IR/Verifier.h"
+#include "LLVMContextImpl.h"
 #include "llvm/ADT/APFloat.h"
 #include "llvm/ADT/APInt.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/MapVector.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallPtrSet.h"
class Verifier : public InstVisitor<Verifier>, VerifierSupport {
 
   // Keeps track of duplicate function argument debug info.
   SmallVector<const DILocalVariable *, 16> DebugFnArgs;
 
   TBAAVerifier TBAAVerifyHelper;
 
   void checkAtomicMemAccessSize(Type *Ty, const Instruction *I);
 
+  using BlockAddressMapTy = decltype(LLVMContextImpl::BlockAddresses);
+
 public:
   explicit Verifier(raw_ostream *OS, bool ShouldTreatBrokenDebugInfoAsError,
                     const Module &M)
       : VerifierSupport(OS, M), LandingPadResultTy(nullptr),
         SawFrameEscape(false), TBAAVerifyHelper(this) {
     TreatBrokenDebugInfoAsError = ShouldTreatBrokenDebugInfoAsError;
   }
 
for (const GlobalAlias &GA : M.aliases())
       visitGlobalAlias(GA);
 
     for (const NamedMDNode &NMD : M.named_metadata())
       visitNamedMDNode(NMD);
 
     for (const StringMapEntry<Comdat> &SMEC : M.getComdatSymbolTable())
       visitComdat(SMEC.getValue());
 
+    verifyBlockAddressConstants(M.getContext().pImpl->BlockAddresses);
+
     visitModuleFlags(M);
     visitModuleIdents(M);
     visitModuleCommandLines(M);
 
     verifyCompileUnits();
 
     verifyDeoptimizeCallingConvs();
     DISubprogramAttachments.clear();
#include "llvm/IR/Metadata.def"
   void verifyCompileUnits();
 
   /// Module-level verification that all @llvm.experimental.deoptimize
   /// declarations share the same calling convention.
   void verifyDeoptimizeCallingConvs();
 
   /// Verify all-or-nothing property of DIFile source attribute within a CU.
   void verifySourceDebugInfo(const DICompileUnit &U, const DIFile &F);
+
+  /// Verify all blockaddresses in \p CM have at least one user.
+  void verifyBlockAddressConstants(const BlockAddressMapTy &CM);
 };
 
 } // end anonymous namespace
 
 /// We know that cond should be true, if not print an error message.
 #define Assert(C, ...) \
   do { if (!(C)) { CheckFailed(__VA_ARGS__); return; } } while (false)
 

 void Verifier::verifySourceDebugInfo(const DICompileUnit &U, const DIFile &F) {
   bool HasSource = F.getSource().hasValue();
   if (!HasSourceDebugInfo.count(&U))
     HasSourceDebugInfo[&U] = HasSource;
   AssertDI(HasSource == HasSourceDebugInfo[&U],
            "inconsistent use of embedded source");
 }
 
+void Verifier::verifyBlockAddressConstants(const BlockAddressMapTy &CM) {
+  for (auto &KV : CM)
+    Assert(!KV.second->use_empty(), "Blockaddress does not have any users",
+           KV.second);
+}
+
 //===----------------------------------------------------------------------===//
 //  Implement the public interfaces to this file...
 //===----------------------------------------------------------------------===//
 
 bool llvm::verifyFunction(const Function &f, raw_ostream *OS) {
   Function &F = const_cast<Function &>(f);
 
   // Don't use a raw_null_ostream.  Printing IR is expensive.