[ConstantHoisting] Ignore unreachable bb:s when collecting candidates

[bjope]

Summary

Ignore looking at blocks that are unreachable from entry when
collecting candidates for hosting.

Normally the consthoist pass is executed in the llc pipeline,
just after unreachableblockelim. So it is abnormal to have code
that is unreachable from the entry block. But when running the
pass as part of opt, for example as part of fuzzy testing, we
might trigger various kinds of asserts when collecting candidates
if we include unreachable blocks in that analysis.

Since it also just is a waste of time trying to hoist constants
in unreachble code, the simple solution is to simply ignore such
blocks when collecting the hoisting candidates.

The two added test cases used to end up in two different asserts,
and the intention with the checks is just to verify that we no
longer fail.

Fixes: PR43903

[bjope]

llvm/test/Transforms/ConstantHoisting/X86/pr43903-not-all-uses-rebased.ll

5

I'll remove the internal ticket number BBI-355111 before landing this.

[spatel]

LGTM

llvm/test/Transforms/ConstantHoisting/X86/pr43903-not-all-uses-rebased.ll

3

Should this RUN line specify the x86 triple explicitly, or it doesn't matter? (not sure what will happen on bots that are non-x86)

[bjope]

llvm/test/Transforms/ConstantHoisting/X86/pr43903-not-all-uses-rebased.ll

3

Ah thanks! I've missed to add the triple here. I'll fix that when landing this.

[spatel]

llvm/test/Transforms/ConstantHoisting/X86/pr43903-not-all-uses-rebased.ll

3

Actually after I posted the comment, I noticed that the triple is explicitly stated under here with "target triple".

So I don't think there's any problem, but it might be nicer to have it in the RUN line for consistency with the other test.

llvm/lib/Transforms/Scalar/ConstantHoisting.cpp

for (unsigned Idx = 0, E = Inst->getNumOperands(); Idx != E; ++Idx) {
     }
   } // end of for all operands
 }
 
 /// Collect all integer constants in the function that cannot be folded
 /// into an instruction itself.
 void ConstantHoistingPass::collectConstantCandidates(Function &Fn) {
   ConstCandMapType ConstCandMap;
-  for (BasicBlock &BB : Fn)
+  for (BasicBlock &BB : Fn) {
+    // Ignore unreachable basic blocks.
+    if (!DT->isReachableFromEntry(&BB))
+      continue;
     for (Instruction &Inst : BB)
       collectConstantCandidates(ConstCandMap, &Inst);
   }
+}
 
 // This helper function is necessary to deal with values that have different
 // bit widths (APInt Operator- does not like that). If the value cannot be
 // represented in uint64 we return an "empty" APInt. This is then interpreted
 // as the value is not in range.
 static Optional<APInt> calculateOffsetDiff(const APInt &V1, const APInt &V2) {
   Optional<APInt> Res = None;
   unsigned BW = V1.getBitWidth() > V2.getBitWidth() ?

llvm/test/Transforms/ConstantHoisting/AArch64/consthoist-unreachable.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -mtriple aarch64-- -consthoist -S | FileCheck %s
+
+; This used to trigger an assertion failure:
+;
+;    ../lib/Transforms/Scalar/ConstantHoisting.cpp:779: void llvm::ConstantHoistingPass::emitBaseConstants(llvm::Instruction *, llvm::Constant *, llvm::Type *, const llvm::consthoist::ConstantUser &): Assertion `CastInst->isCast() && "Expected an cast instruction!"' failed.
+
+@c.a = external global i32, align 1
+
+define void @c() {
+; CHECK-LABEL: @c(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp ne i16 0, 0
+; CHECK-NEXT:    br i1 undef, label [[LBL1_US:%.*]], label [[ENTRY_ENTRY_SPLIT_CRIT_EDGE:%.*]]
+; CHECK:       entry.entry.split_crit_edge:
+; CHECK-NEXT:    [[CONST:%.*]] = bitcast i32 1232131 to i32
+; CHECK-NEXT:    br label [[LBL1:%.*]]
+; CHECK:       lbl1.us:
+; CHECK-NEXT:    [[CONST1:%.*]] = bitcast i32 1232131 to i32
+; CHECK-NEXT:    store i32 [[CONST1]], i32* @c.a, align 1
+; CHECK-NEXT:    br label [[FOR_COND4:%.*]]
+; CHECK:       lbl1:
+; CHECK-NEXT:    store i32 [[CONST]], i32* @c.a, align 1
+; CHECK-NEXT:    br i1 undef, label [[IF_THEN:%.*]], label [[FOR_END12:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    br i1 undef, label [[LBL1]], label [[FOR_COND4]]
+; CHECK:       for.cond4:
+; CHECK-NEXT:    br label [[FOR_COND4]]
+; CHECK:       for.body9:
+; CHECK-NEXT:    store i32 1232131, i32* undef, align 1
+; CHECK-NEXT:    store i32 1232132, i32* undef, align 1
+; CHECK-NEXT:    br label [[FOR_BODY9:%.*]]
+; CHECK:       for.end12:
+; CHECK-NEXT:    ret void
+;
+entry:
+  %tobool = icmp ne i16 0, 0
+  br i1 undef, label %lbl1.us, label %entry.entry.split_crit_edge
+
+entry.entry.split_crit_edge:                      ; preds = %entry
+  br label %lbl1
+
+lbl1.us:                                          ; preds = %entry
+  store i32 1232131, i32* @c.a, align 1
+  br label %for.cond4
+
+lbl1:                                             ; preds = %if.then, %entry.entry.split_crit_edge
+  store i32 1232131, i32* @c.a, align 1
+  br i1 undef, label %if.then, label %for.end12
+
+if.then:                                          ; preds = %lbl1
+  br i1 undef, label %lbl1, label %for.cond4
+
+for.cond4:                                        ; preds = %for.cond4, %if.then, %lbl1.us
+  br label %for.cond4
+
+for.body9:                                        ; preds = %for.body9
+  store i32 1232131, i32* undef, align 1
+  store i32 1232132, i32* undef, align 1
+  br label %for.body9
+
+for.end12:                                        ; preds = %lbl1
+  ret void
+}

llvm/test/Transforms/ConstantHoisting/X86/pr43903-not-all-uses-rebased.ll

+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -mtriple=x86_64-unknown-linux-gnu -consthoist -consthoist-gep=1 -S | FileCheck %s
+
+; This is a reproducer for PR43903 where we hit an assertion:
+; opt: ../lib/Transforms/Scalar/ConstantHoisting.cpp:903: bool llvm::ConstantHoistingPass::emitBaseConstants(llvm::GlobalVariable *): Assertion `UsesNum == (ReBasesNum + NotRebasedNum) && "Not all uses are rebased"' failed.
+
+target triple = "x86_64-unknown-linux-gnu"
+
+@a = external global [2 x i16], align 1
+
+define void @c() {
+; CHECK-LABEL: @c(
+; CHECK-NEXT:  for.cond:
+; CHECK-NEXT:    br i1 undef, label [[FOR_BODY2:%.*]], label [[FOR_END4:%.*]]
+; CHECK:       for.body2:
+; CHECK-NEXT:    br i1 undef, label [[LAND_RHS:%.*]], label [[LAND_END:%.*]]
+; CHECK:       land.rhs:
+; CHECK-NEXT:    unreachable
+; CHECK:       land.end:
+; CHECK-NEXT:    [[CONST1:%.*]] = bitcast i16* getelementptr inbounds ([2 x i16], [2 x i16]* @a, i32 0, i32 0) to i16*
+; CHECK-NEXT:    [[CMP:%.*]] = icmp ule i16* undef, [[CONST1]]
+; CHECK-NEXT:    unreachable
+; CHECK:       for.cond3:
+; CHECK-NEXT:    [[TMP0:%.*]] = load i16, i16* getelementptr inbounds ([2 x i16], [2 x i16]* @a, i32 0, i32 1), align 1
+; CHECK-NEXT:    br label [[FOR_COND3:%.*]]
+; CHECK:       for.end4:
+; CHECK-NEXT:    [[CONST:%.*]] = bitcast i16* getelementptr inbounds ([2 x i16], [2 x i16]* @a, i32 0, i32 0) to i16*
+; CHECK-NEXT:    [[TMP1:%.*]] = load i16, i16* [[CONST]], align 1
+; CHECK-NEXT:    ret void
+;
+for.cond:
+  br i1 undef, label %for.body2, label %for.end4
+
+for.body2:                                        ; preds = %for.cond
+  br i1 undef, label %land.rhs, label %land.end
+
+land.rhs:                                         ; preds = %for.body2
+  unreachable
+
+land.end:                                         ; preds = %for.body2
+  %cmp = icmp ule i16* undef, getelementptr inbounds ([2 x i16], [2 x i16]* @a, i32 0, i32 0)
+  unreachable
+
+for.cond3:                                        ; preds = %for.cond3
+  %tmp0 = load i16, i16* getelementptr inbounds ([2 x i16], [2 x i16]* @a, i32 0, i32 1), align 1
+  br label %for.cond3
+
+for.end4:                                         ; preds = %for.cond
+  %tmp1 = load i16, i16* getelementptr inbounds ([2 x i16], [2 x i16]* @a, i32 0, i32 0), align 1
+  ret void
+}