[PATCH] D107384: Simplify coro::salvageDebugInfo() (NFC-ish)

[Adrian Prantl via Phabricator via llvm-commits]

aprantl marked an inline comment as done.
aprantl added inline comments.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2533-2537
+      Value *Op = llvm::salvageDebugInfoImpl(
+          *Inst, Expr ? Expr->getNumLocationOperands() : 0, Ops,
           AdditionalValues);
-      if (!Storage)
+      if (!Op)
         break;
----------------
ChuanqiXu wrote:
> If `Op` is null and we break this loop, Storage would still be no-nullptr. It means that it wouldn't trigger the condition in 2547, then there could be a potential bug to me.
Previously we filtered out only GEP instructions on line 2530, and so the `if (!Storage)` meant salvaging a GEP instruction failed. In the new code we try to salvage //any// instruction and so the `if (!Op)` mostly takes over the role of the `else break`. In the new code the `else break` only catches function arguments, constants, and other non-instructions.
There's a behavior change here in that we no longer can detect if a GEP instruction was unsalvageable. Note that we still filter for function arguments on line 2558. The new code is now rewriting DVI for a failed GEP, which the old code didn't, but that's a perfectly safe change that doesn't pessimize debug info in any way.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107384/new/

https://reviews.llvm.org/D107384