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

Wed Aug 4 18:41:15 PDT 2021

ChuanqiXu 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;
----------------
aprantl wrote:
> 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.
Many thanks for the detailed explanation! Now I got it. Another question is that: is the line `if (!Storage)` and the loop condition `while(Storage)`  redundant now? Since it looks like that `Storage` wouldn't be converted to null in this loop.

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

https://reviews.llvm.org/D107384