[Reassociate] Do not drop debug location if replacement is missing

[dstenb]

Summary

When reassociating an expression, do not drop the instruction's
original debug location in case the replacement location is
missing.

The debug location must at least not be dropped for inlinable
callsites of debug-info-bearing functions in debug-info-bearing
functions. Failing to do so would result in an "inlinable function "
"call in a function with debug info must have a !dbg location"
error in the verifier.

As preserving the original debug location is not expected
to result in overly jumpy debug line information, it is
preserved for all other cases too.

This fixes PR34231:
https://bugs.llvm.org/show_bug.cgi?id=34231

[aprantl]

I fully understand the rationale behind this patch and I think it is good, but why is it ok/desirable to drop the debug info in all other cases?

test/Transforms/Reassociate/keep-inlinable-fn-dbgloc.ll

1

This test is missing a CHECK (otherwise it would work even after symlinking opt to /bin/true :-)

[dstenb]

In D36865#845632, @aprantl wrote:

I fully understand the rationale behind this patch and I think it is good, but why is it ok/desirable to drop the debug info in all other cases?

Good question!

My original reasoning was that it would be good to drop the debug location in other cases to avoid jumpy line information. Now when I take another look at the pass I'm really starting to doubt if that really is a valid concern. I have not been able to create a reproducer that showcases that problem clearly; however, as I'm not very knowledgeable about the pass, it might as well be due to limitations in my imagination.

It is maybe better to not drop the debug information in the other cases.

[davide]

I don't think we should drop DI in other cases (your reasoning is correct).

lib/Transforms/Scalar/Reassociate.cpp

2101

auto

test/Transforms/Reassociate/keep-inlinable-fn-dbgloc.ll

1

Indeed :) And we have evidence this happened in the past

[dstenb]

Changed so that the debug location is kept in the other cases.
Added CHECK statements (thanks for pointing that out!).

[dstenb]

test/Transforms/Reassociate/keep-inlinable-fn-dbgloc.ll

1

Fixed in the latest patch. Thanks for pointing that out!

[davide]

LGTM

lib/Transforms/Scalar/Reassociate.cpp

   ReassociateExpression(BO);
 }
 
+static bool canDropDebugLoc(Instruction *I) {
+  if (CallInst *CI = dyn_cast<CallInst>(I)) {
+    CallSite CS = CallSite(CI);
+    // Inlinable callsites of debug-info-bearing functions in
+    // debug-info-bearing functions must have debug locations
+    // attached to them. See rationale in the IR verifier.
+    if (CI->getFunction()->getSubprogram() && CS.getCalledFunction() &&
+        CS.getCalledFunction()->getSubprogram())
+      return false;
+  }
+
+  return true;
+}
+
 void ReassociatePass::ReassociateExpression(BinaryOperator *I) {
   // First, walk the expression tree, linearizing the tree, collecting the
   // operand information.

     DEBUG(dbgs() << "Reassoc to scalar: " << *V << '\n');
     I->replaceAllUsesWith(V);
     if (Instruction *VI = dyn_cast<Instruction>(V))
-      VI->setDebugLoc(I->getDebugLoc());
+      if (I->getDebugLoc() || canDropDebugLoc(VI))
+        VI->setDebugLoc(I->getDebugLoc());
     RedoInsts.insert(I);
     ++NumAnnihil;
     return;

test/Transforms/Reassociate/keep-inlinable-fn-dbgloc.ll

+;RUN: opt -S -reassociate -disable-output < %s
+
+; Inlinable callsites of debug-info-bearing functions in
+; debug-info-bearing functions must have debug locations
+; attached to them. Therefore, do not drop debug locations for
+; such instructions when reassociating expressions.
+;
+; This would trigger an "inlinable function call in a function with "
+; "debug info must have a !dbg location" error in the verifier otherwise.
+
+define i16 @fn1() !dbg !3 {
+  ret i16 undef
+}
+
+define void @fn2() !dbg !6 {
+  %inlinable_call = call i16 @fn1(), !dbg !7
+  %dbgless_instruction = or i16 %inlinable_call, 0
+  store i16 %dbgless_instruction, i16* undef, align 1
+  unreachable
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 6.0.0", isOptimized: true, runtimeVersion: 0, emissionKind: LineTablesOnly)
+!1 = !DIFile(filename: "foo.c", directory: "/")
+!2 = !{i32 2, !"Debug Info Version", i32 3}
+!3 = distinct !DISubprogram(name: "fn1", scope: !1, file: !1, line: 2, type: !4, isLocal: false, isDefinition: true, scopeLine: 2, isOptimized: true, unit: !0)
+!4 = !DISubroutineType(types: !5)
+!5 = !{}
+!6 = distinct !DISubprogram(name: "fn2", scope: !1, file: !1, line: 3, type: !4, isLocal: false, isDefinition: true, scopeLine: 3, isOptimized: true, unit: !0)
+!7 = !DILocation(line: 7, column: 10, scope: !6)