Remove dangling BlockAddresses in GlobalDCE

[bruno]

Summary

Whenever globaldce delete globals vars, it updates their initializers to nullptr and leave underlying constants to be cleaned up later by its uses. However, initializers containing dangling BlockAddresses are never removed because its uses (Functions and Basic Blocks) may still be alive. The presence of dead BlockAddress after globaldce is a bug - the target basic block reference count is never updated - and also prevents other optimizations; e.g. simplifycfg to merge blocks.

This patch fix this issue by removing dead BlockAddresses.

[• rafael]

+; RUN: opt -O2 -simplifycfg -inline -early-cse -S < %s | FileCheck %s

Why do you need all these passes? You should be able to use just "
-simplifycfg -inline", no?

[bruno]

Not really. But anyway, this could be indeed improved, the minimum would be:
"opt -ipsccp -instcombine -jump-threading -globaldce -simplifycfg -inline"

[• rafael]

why? The patch only changes GlobalDCE.cpp. Why do you need to run
passes before it?

[bruno]

Without this patch, GlobalDCE won't properly clean up blockaddress references, leaving BBs ref counts without proper update. One way to test that such ref counts are updated is to expose the module to further optimizations that actually use this refcount; simplifycfg in this example.
Other passes need to be executed before GlobalDCE to trigger the removal of @foo.addrs.

[bruno]

I was stuck with the original bug testcase, this is indeed much smaller and simple :-)
Attached a new patch version, thanks Rafael!

[bruno]

Ok to commit?

[• rafael]

One sec, let me review the code itself now that I understand the test :-)

[bruno]

Hi Rafael,

I've tried this path before and the problem I found is exactly what you fix with:

+ if (isa<ConstantInt>(C) || isa<ConstantFP>(C)) // why?
+ return false;
+

I->setInitializer(nullptr) will actually destroy the constant in ConstantInt and ConstantFP and later on there's nothing there for destroyConstant to destroy.

This is not the case for other underlying constant types - which you can safely keep track by saving the initializer and deleting it later. I prefer your approach, but I suggest we do not change isSafeToDestroyConstant, but check this condition before. See the attached patch.

lib/Transforms/IPO/GlobalDCE.cpp

 #include "llvm/IR/Instructions.h"
 #include "llvm/IR/Module.h"
 #include "llvm/Transforms/Utils/CtorUtils.h"
+#include "llvm/Transforms/Utils/GlobalStatus.h"
 #include "llvm/Pass.h"
 using namespace llvm;
 

        I != E; ++I)
     if (!AliveGlobals.count(I)) {
       DeadGlobalVars.push_back(I);         // Keep track of dead globals
-      I->setInitializer(nullptr);
+      if (I->hasInitializer()) {
+        Constant *Init = I->getInitializer();
+        I->setInitializer(nullptr);
+
+        // Setting initializers to nullptr is enough for Int and FP but not
+        // for other underlying constant types
+        bool IsIntOrFPCst = isa<ConstantInt>(Init) || isa<ConstantFP>(Init);
+        if (!IsIntOrFPCst && isSafeToDestroyConstant(Init))
+          Init->destroyConstant();
+      }
     }
 
   // The second pass drops the bodies of functions which are dead...

test/Transforms/GlobalDCE/deadblockaddr.ll

+; RUN: opt -globaldce -simplifycfg -S < %s | FileCheck %s
+
+; Tests whether globaldce does the right cleanup while removing @bar
+; so that a dead BlockAddress reference to foo won't prevent other passes
+; to work properly, e.g. simplifycfg
+@bar = internal unnamed_addr constant i8* blockaddress(@foo, %L1)
+
+; CHECK-LABEL: foo
+; CHECK-NOT: br label %L1
+; CHECK: ret void
+define void @foo() {
+entry:
+  br label %L1
+L1:
+  ret void
+}