[SimplifyCFG] Fix the compile crash for invalid upper bound value

[vfdff]

For '%add = add nuw i32 %x, 1', we can only infer the LowerBound is 1, and keep the UpperBound is initialized 0 in computeConstantRange. so we can't assume the UpperBound is valid bound when its value is 0.

Fix #71329.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Allen (vfdff)

Changes

For '%add = add nuw i32 %x', we can only infer the LowerBound is 1, and keep the UpperBound is initialized 0 in computeConstantRange. so we can't assume the UpperBound is valid bound when its value is 0.

Fix #71329.

---

Full diff: https://github.com/llvm/llvm-project/pull/71351.diff

2 Files Affected:

• (modified) llvm/lib/Transforms/Utils/SimplifyCFG.cpp (+6-3)
• (modified) llvm/test/Transforms/SimplifyCFG/switch_mask.ll (+32)

diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index b58bcd35f940f96..1f068309b61ff57 100644
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -6639,13 +6639,16 @@ static bool SwitchToLookupTable(SwitchInst *SI, IRBuilder<> &Builder,
     // WouldFitInRegister.
     // TODO: Consider growing the table also when it doesn't fit in a register
     // if no optsize is specified.
-    if (all_of(ResultTypes, [&](const auto &KV) {
+    // Check the UpperBound is updated with a valid value.
+    const APInt &UpperBound = CR.getUpper();
+    if (UpperBound.getZExtValue() != 0 &&
+        all_of(ResultTypes, [&](const auto &KV) {
           return SwitchLookupTable::WouldFitInRegister(
-              DL, CR.getUpper().getLimitedValue(), KV.second /* ResultType */);
+              DL, UpperBound.getLimitedValue(), KV.second /* ResultType */);
         })) {
       // The default branch is unreachable after we enlarge the lookup table.
       // Adjust DefaultIsReachable to reuse code path.
-      TableSize = CR.getUpper().getZExtValue();
+      TableSize = UpperBound.getZExtValue();
       DefaultIsReachable = false;
     }
   }
diff --git a/llvm/test/Transforms/SimplifyCFG/switch_mask.ll b/llvm/test/Transforms/SimplifyCFG/switch_mask.ll
index 53ee21eae223c6a..efb8ea507ee63ae 100644
--- a/llvm/test/Transforms/SimplifyCFG/switch_mask.ll
+++ b/llvm/test/Transforms/SimplifyCFG/switch_mask.ll
@@ -173,3 +173,35 @@ lor.end:                                          ; preds = %entry, %entry, %ent
   %0 = phi i1 [ true, %entry ], [ %call, %default ], [ true, %entry ], [ true, %entry ]
   ret i1 %0
 }
+
+; Negative test: The result in default reachable, but its value is not const.
+define void @switch_lookup_with_nonconst_range(i32 %x, i1 %cond) {
+; CHECK-LABEL: @switch_lookup_with_nonconst_range(
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    br label [[FOR_PREHEADER:%.*]]
+; CHECK:       for.preheader:
+; CHECK-NEXT:    br i1 [[COND:%.*]], label [[FOR_PREHEADER]], label [[LOR_END:%.*]]
+; CHECK:       lor.end:
+; CHECK-NEXT:    ret void
+;
+entry:
+  br label %for.preheader
+
+for.preheader:                                    ; preds = %for.preheader, %entry
+  %add = add nuw i32 %x, 1                        ; the UpperBound is unconfirmed
+  br i1 %cond, label %for.preheader, label %for.end
+
+for.end:                                          ; preds = %for.preheader
+  switch i32 %add, label %default [
+  i32 0, label %lor.end
+  i32 1, label %lor.end
+  i32 5, label %lor.end
+  ]
+
+default:                                          ; preds = %for.end
+  br label %lor.end
+
+lor.end:                                          ; preds = %default, %for.end, %for.end, %for.end
+  %retval.0.i.i = phi i32 [ 0, %default ], [ 0, %for.end ], [ 0, %for.end ], [ 0, %for.end ]
+  ret void
+}

[vfdff]

I didn't find an API, which check whether the Upper of return value of computeConstantRange is updated, so I test it with UpperBound.getZExtValue() != 0

[zmodem]

Since @nikic is the ConstantRange expert, I'd like his input here.

Please let me know if I've got this right. In the test we have:

%add = add nuw i32 %x, 1                        ; the UpperBound is unconfirmed

It seems the upper bound for %add would be 2^32-1. However, since the upper end of ConstantRange is exclusive, it means ConstantRange::Upper becomes 2^32 which wraps to zero? That's pretty subtle.

Instead of checking for zero, I think this would be clearer:

// Check that the constant range hasn't wrapped.
if (CR.getUpper() > CR.getLower() && ...

[nikic]

It's probably best to use getUnsignedMax() and then do +1 when assigning TableSize. getUnsignedMax() is the inclusive unsigned upper bound (UINT_MAX for a wrapped range).

[vfdff]

Thanks for @nikic and @zmodem.

If the type of TableSize is uint64_t （uint64_t TableSize），then it will still wrap for the uint64_t type index of switch ?
So ignore it now with CR.getUpper() > CR.getLower() to quickly fix?

TODO：Refactor the TableSize with type APInt to relax the restriction.

[vfdff]

Update and use UpperBound.getZExtValue() > CR.getLower().getZExtValue() to avoid involving wrap.

[zmodem]

Why getLimitedValue() here but getZextValue() everywhere else?

[vfdff]

Oh, thanks, I'll unify the update to getLimitedValue()

[nikic]

Suggested change

	if (UpperBound.getZExtValue() > CR.getLower().getZExtValue() &&
	if (!CR.isUpperWrapped() &&

[vfdff]

Apply your comment, thanks

[nikic]

LGTM as well.