[Evaluator] Improve evaluation of load/store

[evgeny777]

Summary

When storing pointers in struct/class constructor clang often generates IR like this:

define internal void @_GLOBAL__sub_I_main.cpp()  {
  %1 = load i64, i64* bitcast (%struct.A** @gA to i64*), align 8
  store i64 %1, i64* bitcast (%struct.S* @s to i64*), align 8
  ret void
}

which corresponds to following C++ code:

extern A *gA;
struct S {
  A* _a;
} SObj(gA);

With D43077 it is now possible to import variables from different TU when using ThinLTO,
so it would be nice to optimize such static ctors out.

[evgeny777]

Any comments on this?

[tejohnson]

Sorry for the delay. I don't feel super confident reviewing these change. I looked back at who touched the ComputeLoadResult code last (besides pcc who simply extracted it out of GlobalOpt into Utils), and it was Chris Lattner in 2005! So I am not completely sure who to add to as a reviewer at this point.
Poking around LLVM though, I see that there is some very similar handling in llvm::ConstantFoldLoadFromConstPtr (ConstantFolding.cpp). In particular, the handling of P as a GlobalVariable or GEP appears identical to what is already in Evaluator::ComputeLoadResult. It already has handling for bitcasts, although that handling appears quite a bit more complex that what you added to ComputeLoadResult. I'm wondering whether ComputeLoadResult can be refactored to just call ConstantFoldLoadFromConstPtr, or whether there is a reason it needs to remain distinct (and why the bitcast handling added here is simpler).

[evgeny777]

Unfortunately ConstantFoldLoadFromConstPtr seems to be useful, because it checks for GV initializer being constant and we're not forced by this constraint when evaluating static ctors.
However it turned out we can use ConstantFoldLoadThroughBitcast when evaluating both load and store.

[tejohnson]

LGTM with some minor comments below.

include/llvm/Analysis/ConstantFolding.h

150 ↗

(On Diff #137443)

document

lib/Transforms/Utils/Evaluator.cpp

202

The two opcode cases are both doing the same checks (is it a global variable with a definitive initializer), but the checking is structured differently. Suggest structuring the checks the same way for both opcodes.

lib/Transforms/Utils/Evaluator.cpp

Constant *Evaluator::ComputeLoadResult(Constant *P) {
   // Access it.
   if (GlobalVariable *GV = dyn_cast<GlobalVariable>(P)) {
     if (GV->hasDefinitiveInitializer())
       return GV->getInitializer();
     return nullptr;
   }
 
   // Handle a constantexpr getelementptr.
-  if (ConstantExpr *CE = dyn_cast<ConstantExpr>(P))
+  if (ConstantExpr *CE = dyn_cast<ConstantExpr>(P)) {
     if (CE->getOpcode() == Instruction::GetElementPtr &&
         isa<GlobalVariable>(CE->getOperand(0))) {
       GlobalVariable *GV = cast<GlobalVariable>(CE->getOperand(0));
       if (GV->hasDefinitiveInitializer())
         return ConstantFoldLoadThroughGEPConstantExpr(GV->getInitializer(), CE);
-    }
+    } else if (CE->getOpcode() == Instruction::BitCast) {
+      GlobalVariable *GV = dyn_cast<GlobalVariable>(CE->getOperand(0));
+      if (GV && GV->hasDefinitiveInitializer()) {
+        // We should always load from a pointer
+        assert(P->getType()->isPointerTy());
+        auto *ResTy = P->getType()->getContainedType(0);
+        auto *I = GV->getInitializer();
+        if (I->getType()->isPointerTy() && ResTy->isIntegerTy())
+          return ConstantExpr::getPtrToInt(I, ResTy);
+      }
+    }
 
   return nullptr;  // don't know how to evaluate.
 }
 
 /// Evaluate all instructions in block BB, returning true if successful, false
 /// if we can't evaluate it.  NewBB returns the next BB that control flows into,
 /// or null upon return.
 bool Evaluator::EvaluateBlock(BasicBlock::iterator CurInst,
if (StoreInst *SI = dyn_cast<StoreInst>(CurInst)) {
           DEBUG(dbgs() << "Attempting to resolve bitcast on constant ptr.\n");
           // If we're evaluating a store through a bitcast, then we need
           // to pull the bitcast off the pointer type and push it onto the
           // stored value.
           Ptr = CE->getOperand(0);
 
           Type *NewTy = cast<PointerType>(Ptr->getType())->getElementType();
 
+          auto TypeConversionSafe = [](Type *VTy, Type *Ty) {
+            return (VTy->isIntegerTy() && Ty->isPointerTy()) ||
+                   VTy->canLosslesslyBitCastTo(Ty);
+          };
           // In order to push the bitcast onto the stored value, a bitcast
           // from NewTy to Val's type must be legal.  If it's not, we can try
           // introspecting NewTy to find a legal conversion.
-          while (!Val->getType()->canLosslesslyBitCastTo(NewTy)) {
+          auto *VTy = Val->getType();
+          while (!TypeConversionSafe(VTy, NewTy)) {
             // If NewTy is a struct, we can convert the pointer to the struct
             // into a pointer to its first member.
             // FIXME: This could be extended to support arrays as well.
             if (StructType *STy = dyn_cast<StructType>(NewTy)) {
               NewTy = STy->getTypeAtIndex(0U);
 
               IntegerType *IdxTy = IntegerType::get(NewTy->getContext(), 32);
               Constant *IdxZero = ConstantInt::get(IdxTy, 0, false);
               Constant * const IdxList[] = {IdxZero, IdxZero};
 
               Ptr = ConstantExpr::getGetElementPtr(nullptr, Ptr, IdxList);
               if (auto *FoldedPtr = ConstantFoldConstant(Ptr, DL, TLI))
                 Ptr = FoldedPtr;
 
             // If we can't improve the situation by introspecting NewTy,
             // we have to give up.
             } else {
               DEBUG(dbgs() << "Failed to bitcast constant ptr, can not "
                     "evaluate.\n");
               return false;
             }
           }
 
           // If we found compatible types, go ahead and push the bitcast
-          // onto the stored value.
-          Val = ConstantExpr::getBitCast(Val, NewTy);
+          // onto the stored value or convert it from integer to pointer.
+          Val = VTy->isIntegerTy() ? ConstantExpr::getIntToPtr(Val, NewTy)
+                                   : ConstantExpr::getBitCast(Val, NewTy);
 
           DEBUG(dbgs() << "Evaluated bitcast: " << *Val << "\n");
         }
       }
 
       MutatedMemory[Ptr] = Val;
     } else if (BinaryOperator *BO = dyn_cast<BinaryOperator>(CurInst)) {
       InstResult = ConstantExpr::get(BO->getOpcode(),

test/Transforms/GlobalOpt/evaluate-bitcast.ll

+; RUN: opt -globalopt -instcombine %s -S -o - | FileCheck %s
+
+; Static constructor should have been optimized out
+; CHECK:       i32 @main
+; CHECK-NEXT:     ret i32 69905
+; CHECK-NOT:   _GLOBAL__sub_I_main.cpp
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-linux-gnu"
+
+%struct.S = type { %struct.A* }
+%struct.A = type { i64, i64 }
+
+@s = internal local_unnamed_addr global %struct.S zeroinitializer, align 8
[e-mail]_ctors = appending global [1 x { i32, void ()*, i8* }] [{ i32, void ()*, i8* } { i32 65535, void ()* @_GLOBAL__sub_I_main.cpp, i8* null }]
+@gA = available_externally dso_local local_unnamed_addr global %struct.A* inttoptr (i64 69905 to %struct.A*), align 8
+
+define dso_local i32 @main() local_unnamed_addr {
+  %1 = load i64, i64* bitcast (%struct.S* @s to i64*), align 8
+  %2 = trunc i64 %1 to i32
+  ret i32 %2
+}
+
+define internal void @_GLOBAL__sub_I_main.cpp() section ".text.startup" {
+  %1 = load i64, i64* bitcast (%struct.A** @gA to i64*), align 8
+  store i64 %1, i64* bitcast (%struct.S* @s to i64*), align 8
+  ret void
+}