[LowerTypeTests] Avoid creation of select constant expression

[nikic]

Summary

LowerTypeTests replaces weak declarations with an icmp+select constant expressions. As this is not a relocatable expression, it additionally promotes initializers using it to global ctors.

As part of https://discourse.llvm.org/t/rfc-remove-most-constant-expressions/63179, I would like to remove the select constant expression, of which LTT is now the last user. This is a bit tricky, because we now need to replace a constant with an instruction, which might require converting intermediate constant expression users to instructions as well.

We do this using the convertUsersOfConstantsToInstructions() helper. However, it needs to be slightly extended to also support expansion of ConstantAggregates. These are important in this context, because the promotion of initializers to global ctors will produce stores of such aggregates.

[zequanwu]

Hi, it causes broken module error when building media_unittests in chromium project (https://bugs.chromium.org/p/chromium/issues/detail?id=1423915 for detail). I'll work on a reduced repo later.

[hans]

From just a quick look, I guess what might be happening is this transformation replacing a select constant expression in a PHI node, and inserting the replacement instruction(s) above the PHI node, which the verifier doesn't like.

[nikic]

I've reapplied this change in rG5b86eaeb7e4d0b508e8dc9592b63361c5abb1e48 with fixed handling for uses in phis. Let me know if you still see issues.

llvm/lib/IR/ReplaceConstant.cpp

for (Use &U : I->operands()) {
     }
 
     // Associate all the collected paths with U, and save it.
     if (!Paths.empty())
       CEPaths[&U] = Paths;
   }
 }
 
+static bool isExpandableUser(User *U) {
+  return isa<ConstantExpr>(U) || isa<ConstantAggregate>(U);
+}
+
+static Instruction *expandUser(Instruction *InsertPt, Constant *C) {
+  if (auto *CE = dyn_cast<ConstantExpr>(C)) {
+    return CE->getAsInstruction(InsertPt);
+  } else if (isa<ConstantStruct>(C) || isa<ConstantArray>(C)) {
+    Value *V = PoisonValue::get(C->getType());
+    for (auto [Idx, Op] : enumerate(C->operands()))
+      V = InsertValueInst::Create(V, Op, Idx, "", InsertPt);
+    return cast<Instruction>(V);
+  } else if (isa<ConstantVector>(C)) {
+    Type *IdxTy = Type::getInt32Ty(C->getContext());
+    Value *V = PoisonValue::get(C->getType());
+    for (auto [Idx, Op] : enumerate(C->operands()))
+      V = InsertElementInst::Create(V, Op, ConstantInt::get(IdxTy, Idx), "",
+                                    InsertPt);
+    return cast<Instruction>(V);
+  } else {
+    llvm_unreachable("Not an expandable user");
+  }
+}
+
 bool convertUsersOfConstantsToInstructions(ArrayRef<Constant *> Consts) {
-  // Find all ConstantExpr that are direct users Consts.
-  SmallVector<ConstantExpr *> Stack;
+  // Find all expandable direct users of Consts.
+  SmallVector<Constant *> Stack;
   for (Constant *C : Consts)
     for (User *U : C->users())
-      if (ConstantExpr *CE = dyn_cast<ConstantExpr>(U))
-        Stack.push_back(CE);
+      if (isExpandableUser(U))
+        Stack.push_back(cast<Constant>(U));
 
-  // Expand to include constexpr users of direct users
-  SetVector<ConstantExpr *> ConstExprUsers;
+  // Include transitive users.
+  SetVector<Constant *> ExpandableUsers;
   while (!Stack.empty()) {
-    ConstantExpr *V = Stack.pop_back_val();
-    if (ConstExprUsers.contains(V))
+    Constant *C = Stack.pop_back_val();
+    if (!ExpandableUsers.insert(C))
       continue;
 
-    ConstExprUsers.insert(V);
-
-    for (auto *Nested : V->users())
-      if (ConstantExpr *CE = dyn_cast<ConstantExpr>(Nested))
-        Stack.push_back(CE);
+    for (auto *Nested : C->users())
+      if (isExpandableUser(Nested))
+        Stack.push_back(cast<Constant>(Nested));
   }
 
-  // Find all instructions that use any of the ConstExpr users
+  // Find all instructions that use any of the expandable users
   SetVector<Instruction *> InstructionWorklist;
-  for (ConstantExpr *CE : ConstExprUsers)
-    for (User *U : CE->users())
+  for (Constant *C : ExpandableUsers)
+    for (User *U : C->users())
       if (auto *I = dyn_cast<Instruction>(U))
         InstructionWorklist.insert(I);
 
-  // Replace those ConstExpr operands with instructions
+  // Replace those expandable operands with instructions
   bool Changed = false;
   while (!InstructionWorklist.empty()) {
     Instruction *I = InstructionWorklist.pop_back_val();
     for (Use &U : I->operands()) {
       auto *BI = I;
       if (auto *Phi = dyn_cast<PHINode>(I)) {
         BasicBlock *BB = Phi->getIncomingBlock(U);
         BasicBlock::iterator It = BB->getFirstInsertionPt();
         assert(It != BB->end() && "Unexpected empty basic block");
         BI = &*It;
       }
 
-      if (ConstantExpr *C = dyn_cast<ConstantExpr>(U.get())) {
-        if (ConstExprUsers.contains(C)) {
+      if (auto *C = dyn_cast<Constant>(U.get())) {
+        if (ExpandableUsers.contains(C)) {
           Changed = true;
-          Instruction *NI = C->getAsInstruction(BI);
+          Instruction *NI = expandUser(BI, C);
           InstructionWorklist.insert(NI);
           U.set(NI);
-          C->removeDeadConstantUsers();
         }
       }
     }
   }
 
+  for (Constant *C : Consts)
+    C->removeDeadConstantUsers();
+
   return Changed;
 }
 
 } // namespace llvm

llvm/lib/Transforms/IPO/LowerTypeTests.cpp

 #include "llvm/IR/Intrinsics.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/Module.h"
 #include "llvm/IR/ModuleSummaryIndex.h"
 #include "llvm/IR/ModuleSummaryIndexYAML.h"
 #include "llvm/IR/Operator.h"
 #include "llvm/IR/PassManager.h"
+#include "llvm/IR/ReplaceConstant.h"
 #include "llvm/IR/Type.h"
 #include "llvm/IR/Use.h"
 #include "llvm/IR/User.h"
 #include "llvm/IR/Value.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Pass.h"
 #include "llvm/Support/Allocator.h"
 #include "llvm/Support/Casting.h"
void LowerTypeTestsModule::replaceWeakDeclarationWithJumpTablePtr(
   // Can not RAUW F with an expression that uses F. Replace with a temporary
   // placeholder first.
   Function *PlaceholderFn =
       Function::Create(cast<FunctionType>(F->getValueType()),
                        GlobalValue::ExternalWeakLinkage,
                        F->getAddressSpace(), "", &M);
   replaceCfiUses(F, PlaceholderFn, IsJumpTableCanonical);
 
-  Constant *Target = ConstantExpr::getSelect(
-      ConstantExpr::getICmp(CmpInst::ICMP_NE, F,
-                            Constant::getNullValue(F->getType())),
-      JT, Constant::getNullValue(F->getType()));
-  PlaceholderFn->replaceAllUsesWith(Target);
+  convertUsersOfConstantsToInstructions(PlaceholderFn);
+  for (Use &U : make_early_inc_range(PlaceholderFn->uses())) {
+    auto *I = dyn_cast<Instruction>(U.getUser());
+    assert(I && "Non-instruction users should have been eliminated");
+    IRBuilder Builder(I);
+    Value *ICmp = Builder.CreateICmp(CmpInst::ICMP_NE, F,
+                                     Constant::getNullValue(F->getType()));
+    Value *Select = Builder.CreateSelect(ICmp, JT,
+                                         Constant::getNullValue(F->getType()));
+    U.set(Select);
+  }
   PlaceholderFn->eraseFromParent();
 }
 
 static bool isThumbFunction(Function *F, Triple::ArchType ModuleArch) {
   Attribute TFAttr = F->getFnAttribute("target-features");
   if (TFAttr.isValid()) {
     SmallVector<StringRef, 6> Features;
     TFAttr.getValueAsString().split(Features, ',');

llvm/test/Transforms/LowerTypeTests/cfi-direct-call1.ll

 
 ; There are no indirect calls of local_func3 type, it should not be renamed
 ; FULL: define hidden i32 @local_func3(i32 %i)
 
 ; Indirect references to local_func1 and local_func2 must to through jump table
 ; FULL: %fptr1 = select i1 %cmp.i, ptr @local_func1, ptr @local_func2
 
 ; Indirect references to extern_weak and extern_decl must go through jump table
-; FULL: %fptr2 = select i1 %cmp.i, ptr select (i1 icmp ne (ptr @extern_weak, ptr null), ptr getelementptr inbounds ([4 x [8 x i8]], ptr @.cfi.jumptable, i64 0, i64 2), ptr null), ptr getelementptr inbounds ([4 x [8 x i8]], ptr @.cfi.jumptable, i64 0, i64 3)
+; FULL: %0 = select i1 icmp ne (ptr @extern_weak, ptr null), ptr getelementptr inbounds ([4 x [8 x i8]], ptr @.cfi.jumptable, i64 0, i64 2), ptr null
+; FULL: %fptr2 = select i1 %cmp.i, ptr %0, ptr getelementptr inbounds ([4 x [8 x i8]], ptr @.cfi.jumptable, i64 0, i64 3)
 
 ; Direct calls to extern_weak and extern_decl should go to original names
 ; FULL: %call5 = tail call i32 @extern_decl()
 ; FULL: %call7 = tail call i32 @extern_weak()
 
 ; Direct call to local_func1 should to the renamed version
 ; FULL: %call6 = tail call i32 @local_func1.cfi()
 

 ; THIN: define hidden i32 @local_func1.cfi()
 ; THIN: define hidden i32 @local_func2.cfi()
 ; THIN: define hidden i32 @local_func3.cfi(i32 %i){{.*}}
 
 ; Indirect references to local_func1 and local_func2 must to through jump table
 ; THIN: %fptr1 = select i1 %cmp.i, ptr @local_func1, ptr @local_func2
 
 ; Indirect references to extern_weak and extern_decl must go through jump table
-; THIN: %fptr2 = select i1 %cmp.i, ptr select (i1 icmp ne (ptr @extern_weak, ptr null), ptr @extern_weak.cfi_jt, ptr null), ptr @extern_decl.cfi_jt
+; THIN: %0 = select i1 icmp ne (ptr @extern_weak, ptr null), ptr @extern_weak.cfi_jt, ptr null
+; THIN: %fptr2 = select i1 %cmp.i, ptr %0, ptr @extern_decl.cfi_jt
 
 ; Direct calls to extern_weak and extern_decl should go to original names
 ; THIN: %call5 = tail call i32 @extern_decl()
 ; THIN: %call7 = tail call i32 @extern_weak()
 
 ; Direct call to local_func1 should to the renamed version
 ; THIN: %call6 = tail call i32 @local_func1.cfi()
 ; THIN: %call8 = tail call i32 @local_func3.cfi(i32 4)
 

llvm/test/Transforms/LowerTypeTests/function-weak.ll

 ; CHECK:  @llvm.global_ctors = appending global {{.*}}{ i32 0, ptr @__cfi_global_var_init
 
 ; CHECK: declare !type !0 extern_weak void @f()
 declare !type !0 extern_weak void @f()
 
 ; CHECK: define zeroext i1 @check_f()
 define zeroext i1 @check_f() {
 entry:
-; CHECK: ret i1 icmp ne (ptr select (i1 icmp ne (ptr @f, ptr null), ptr @[[JT:.*]], ptr null), ptr null)
+; CHECK: %0 = select i1 icmp ne (ptr @f, ptr null), ptr @[[JT:.*]], ptr null
+; CHECK: %1 = icmp ne ptr %0, null
+; ret i1 %1
   ret i1 icmp ne (ptr @f, ptr null)
 }
 
 ; CHECK: define void @call_f() {
 define void @call_f() {
 entry:
 ; CHECK: call void @f()
   call void @f()
   ret void
 }
 
 declare i1 @llvm.type.test(ptr %ptr, metadata %bitset) nounwind readnone
 
 define i1 @foo(ptr %p) {
   %x = call i1 @llvm.type.test(ptr %p, metadata !"typeid1")
   ret i1 %x
 }
 
 ; X86: define private void @[[JT]]() #{{.*}} align 8 {
 ; ARM: define private void @[[JT]]() #{{.*}} align 4 {
 ; RISCV: define private void @[[JT]]() #{{.*}} align 8 {
 
 ; CHECK: define internal void @__cfi_global_var_init() section ".text.startup" {
 ; CHECK-NEXT: entry:
-; CHECK-NEXT: store ptr select (i1 icmp ne (ptr @f, ptr null), ptr @[[JT]], ptr null), ptr @x, align 8
-; CHECK-NEXT: store ptr select (i1 icmp ne (ptr @f, ptr null), ptr @[[JT]], ptr null), ptr @x2, align 8
-; CHECK-NEXT: store ptr select (i1 icmp ne (ptr @f, ptr null), ptr @[[JT]], ptr null), ptr @x3, align 8
-; CHECK-NEXT: store ptr getelementptr (i8, ptr select (i1 icmp ne (ptr @f, ptr null), ptr @[[JT]], ptr null), i64 42), ptr @x4, align 8
-; CHECK-NEXT: store { ptr, ptr, i32 } { ptr select (i1 icmp ne (ptr @f, ptr null), ptr @[[JT]], ptr null), ptr select (i1 icmp ne (ptr @f, ptr null), ptr @[[JT]], ptr null), i32 42 }, ptr @s, align 8
+; CHECK-NEXT: %0 = select i1 icmp ne (ptr @f, ptr null), ptr @[[JT]], ptr null
+; CHECK-NEXT: store ptr %0, ptr @x, align 8
+; CHECK-NEXT: %1 = select i1 icmp ne (ptr @f, ptr null), ptr @[[JT]], ptr null
+; CHECK-NEXT: store ptr %1, ptr @x2, align 8
+; CHECK-NEXT: %2 = select i1 icmp ne (ptr @f, ptr null), ptr @[[JT]], ptr null
+; CHECK-NEXT: store ptr %2, ptr @x3, align 8
+; CHECK-NEXT: %3 = select i1 icmp ne (ptr @f, ptr null), ptr @[[JT]], ptr null
+; CHECK-NEXT: %4 = getelementptr i8, ptr %3, i64 42
+; CHECK-NEXT: store ptr %4, ptr @x4, align 8
+; CHECK-NEXT: %5 = select i1 icmp ne (ptr @f, ptr null), ptr @[[JT]], ptr null
+; CHECK-NEXT: %6 = insertvalue { ptr, ptr, i32 } poison, ptr %5, 0
+; CHECK-NEXT: %7 = select i1 icmp ne (ptr @f, ptr null), ptr @[[JT]], ptr null
+; CHECK-NEXT: %8 = insertvalue { ptr, ptr, i32 } %6, ptr %7, 1
+; CHECK-NEXT: %9 = insertvalue { ptr, ptr, i32 } %8, i32 42, 2
+; CHECK-NEXT: store { ptr, ptr, i32 } %9, ptr @s, align 8
 ; CHECK-NEXT: ret void
 ; CHECK-NEXT: }
 
 !0 = !{i32 0, !"typeid1"}