[PATCH] D58042: [LiveDebugValues] Emit parameter's entry value

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 5 inline comments as done.
djtodoro added inline comments.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:455
+    const MachineInstr *CurrDebugInstr = &VarLocIDs[ID].MI;
+    auto DebugInstr = std::find_if(DebugEntryVals.begin(), DebugEntryVals.end(),
+                            [&CurrDebugInstr](MachineInstr *MII) {
----------------
aprantl wrote:
> This is going into (low) n^2 territory here. Should we make DebugEntryVals a SmallPtrSet to avoid catastrophic behavior for functions with many arguments?
Mmm, we see.. But we will need another set for caching parameters debug variables, since we consider a parameters `DBG_VALUE` as a "new" if didn't encountered a debug variable from it.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:885
     const VarLoc &DiffIt = VarLocIDs[ID];
-    const MachineInstr *DMI = &DiffIt.MI;
+    const MachineInstr *DebugInstr = &DiffIt.MI;
     MachineInstr *MI =
----------------
aprantl wrote:
> Can you land this as an NFC refactoring (no need for a separate phab review?)
Sure.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:950
+  SmallVector<MachineInstr *, 8> DebugEntryVals;
+  auto IsNewParameter = [&DebugEntryVals](const MachineInstr &MI) -> bool {
+    return !std::any_of(DebugEntryVals.begin(), DebugEntryVals.begin(),
----------------
aprantl wrote:
> What does "New" mean here?
It means that a parameter is not collected yet inside the `DebugEntryVals`. We should name it `IsParamEntryValueCollected` or something like that.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:966
+    if (MI.isDebugValue() && IsUnmodifiedFuncParam(MI) &&
+        IsRegOtherThanSPAndFP(MI.getOperand(0)) && IsNewParameter(MI) &&
+        !IsFragment(MI.getDebugExpression()))
----------------
aprantl wrote:
> We need a longer comment here that explains why each of these conditions are necessary and what is being checked for.
Sure. We will add that.


================
Comment at: lib/CodeGen/LiveDebugValues.cpp:967
+        IsRegOtherThanSPAndFP(MI.getOperand(0)) && IsNewParameter(MI) &&
+        !IsFragment(MI.getDebugExpression()))
+      DebugEntryVals.push_back(&MI);
----------------
aprantl wrote:
> Why can't fragments be entry values?
Fragments can be entry values as well, but don't support it in this initial set of patches.
We will leave a `TODO` for that here.


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

https://reviews.llvm.org/D58042