[DebugInfo][LoopStrengthReduction] Avoid crashes when translating SCEVs containing large integers

[chrisjackson]

Summary

SCEV-based salvaging in LSR translates SCEVs to DIExpressions. SCEVs may
contain very large integers but the translation does not support >64-bit
integers. This patch adds checks to ensure conversions of these large
integers are not attempted.

A regression test, using the repro provided by @JosephTremoulet has also
been added to ensure no such translation is attempted.

PR: https://bugs.llvm.org/show_bug.cgi?id=51329

[chrisjackson]

@JosephTremoulet if the revision goes in, are you happy for me to use your repro as the test, and would you like a credit in the commit?

[StephenTozer]

The implementation looks good to me, the test looks a little funky (see inline comment).

llvm/test/Transforms/LoopStrengthReduce/pr51329.ll

27

Is there a reason the dbg.value in this test uses undef? It looks like a test that logically shouldn't trigger any salvaging behaviour, and probably a bug that needs fixing if the test does work. If this is intended behaviour in some way, it could probably do with a comment at least.

[chrisjackson]

In D107438#2925008, @StephenTozer wrote:

The implementation looks good to me, the test looks a little funky (see inline comment).

Yes I spotted this, and will add an additional patch to prevent the dbg.val being cached for salvaging.

[chrisjackson]

Updated test so that the salvage of an undef'd dbg.value is not attempted. A patch will follow to ensure such salvages are never attempted.

[chrisjackson]

In D107438#2925053, @chrisjackson wrote:

Updated test so that the salvage of an undef'd dbg.value is not attempted. A patch will follow to ensure such salvages are never attempted.

Please see D107448 for this patch,

[JosephTremoulet]

Thanks for the quick fix!

@JosephTremoulet if the revision goes in, are you happy for me to use your repro as the test, and would you like a credit in the commit?

Yes, happy for you to use the repro for the test. No attribution necessary.

Is there a reason the dbg.value in this test uses undef?

That wasn't deliberate on my part, just what popped out from the failure in our rolling tests plus bugpoint.

[StephenTozer]

Changes LGTM.

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

void pushValue(llvm::Value *V) {
     Expr.push_back(ArgIndex);
   }
 
   void pushValue(const SCEVUnknown *U) {
     llvm::Value *V = cast<SCEVUnknown>(U)->getValue();
     pushValue(V);
   }
 
-  void pushConst(const SCEVConstant *C) {
+  bool pushConst(const SCEVConstant *C) {
+    if (C->getAPInt().getMinSignedBits() > 64)
+      return false;
     Expr.push_back(llvm::dwarf::DW_OP_consts);
     Expr.push_back(C->getAPInt().getSExtValue());
+    return true;
   }
 
   /// Several SCEV types are sequences of the same arithmetic operator applied
   /// to constants and values that may be extended or truncated.
   bool pushArithmeticExpr(const llvm::SCEVCommutativeExpr *CommExpr,
                           uint64_t DwarfOp) {
     assert((isa<llvm::SCEVAddExpr>(CommExpr) || isa<SCEVMulExpr>(CommExpr)) &&
            "Expected arithmetic SCEV type");
for (const auto &Op : CastOps)
       pushOperator(Op);
     return Success;
   }
 
   // TODO: MinMax - although these haven't been encountered in the test suite.
   bool pushSCEV(const llvm::SCEV *S) {
     bool Success = true;
     if (const SCEVConstant *StartInt = dyn_cast<SCEVConstant>(S)) {
-      pushConst(StartInt);
+      Success &= pushConst(StartInt);
 
     } else if (const SCEVUnknown *U = dyn_cast<SCEVUnknown>(S)) {
       if (!U->getValue())
         return false;
       pushValue(U->getValue());
 
     } else if (const SCEVMulExpr *MulRec = dyn_cast<SCEVMulExpr>(S)) {
       Success &= pushArithmeticExpr(MulRec, llvm::dwarf::DW_OP_mul);
if (Values.size() == 1 && Expr[0] == llvm::dwarf::DW_OP_LLVM_arg &&
       setFinalExpression(DI, OldExpr);
     }
   }
 
   /// Return true if the combination of arithmetic operator and underlying
   /// SCEV constant value is an identity function.
   bool isIdentityFunction(uint64_t Op, const SCEV *S) {
     if (const SCEVConstant *C = dyn_cast<SCEVConstant>(S)) {
+      if (C->getAPInt().getMinSignedBits() > 64)
+        return false;
       int64_t I = C->getAPInt().getSExtValue();
       switch (Op) {
       case llvm::dwarf::DW_OP_plus:
       case llvm::dwarf::DW_OP_minus:
         return I == 0;
       case llvm::dwarf::DW_OP_mul:
       case llvm::dwarf::DW_OP_div:
         return I == 1;

llvm/test/Transforms/LoopStrengthReduce/pr51329.ll

+; RUN: opt -S -loop-reduce %s | FileCheck %s
+;
+; Test that LSR SCEV-based salvaging does not crash when translating SCEVs
+; that contain integers with binary representations greater than 64-bits. 
+;
+; CHECK: call void @llvm.dbg.value(metadata i64 %var2, metadata !{{[0-9]+}}, metadata !DIExpression(DW_OP_plus_uconst, 228, DW_OP_stack_value))
+
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nofree nosync nounwind readnone speculatable willreturn
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+; Function Attrs: nounwind
+define hidden void @reproducer() local_unnamed_addr !dbg !5 {
+init:
+  %0 = lshr i128 undef, 64
+  %var1 = trunc i128 %0 to i64
+  %1 = add nuw i64 undef, %var1
+  %var2 = lshr i64 %1, 12
+  br label %Label_d0
+
+Label_d0:                                         ; preds = %Label_d0, %init
+  %var3 = phi i64 [ %var2, %init ], [ %var4, %Label_d0 ]
+  call void @llvm.dbg.value(metadata i64 %var2, metadata !11, metadata !DIExpression(DW_OP_plus_uconst, 228, DW_OP_stack_value)), !dbg !12
+  %var4 = add i64 %var3, -1
+  %var5 = icmp eq i64 %var4, 0
+  br i1 %var5, label %Label_1bc, label %Label_d0
+
+Label_1bc:                                        ; preds = %Label_d0
+  ret void
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "frontend", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !2)
+!1 = !DIFile(filename: "source", directory: "")
+!2 = !{}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = distinct !DISubprogram(name: "reproducer", scope: !1, file: !1, line: 904320, type: !6, scopeLine: 904320, spFlags: DISPFlagDefinition, unit: !0, retainedNodes: !10)
+!6 = !DISubroutineType(types: !7)
+!7 = !{null, !8, !9, !9, !9, !9, !9, !9}
+!8 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: null, size: 64)
+!9 = !DIBasicType(name: "my_type", size: 64, encoding: DW_ATE_unsigned)
+!10 = !{!11}
+!11 = !DILocalVariable(name: "my_var", arg: 1, scope: !5, file: !1, line: 904320, type: !8)
+!12 = !DILocation(line: 904544, scope: !5)