[PATCH] D68209: [LiveDebugValues] Introduce entry values of unmodified params

[Jeremy Morse via Phabricator via llvm-commits]

jmorse accepted this revision.
jmorse added a comment.
This revision is now accepted and ready to land.

IMO, this LGTM, with the rider that I think the "empty" method needs to be made safer (see inline).

Keeping the main / "backup" locations starts introducing a hierarchy of longevity for locations, which is where I think we want to be heading.



================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:388
+        return Loc.RegNo;
+      return 0;
+    }
----------------
djtodoro wrote:
> jmorse wrote:
> > Seeing how VarLocIDs is just a UniqueVector, isn't zero a legitimate index? I'm not aware of the first element being initialized to anything specific.
> I this this comments stands for `getEntryValueBackup()`, and I have addressed it. Thanks!
Reading the comments from UniqueVector.h:

  /// insert - Append entry to the vector if it doesn't already exist.  Returns
  /// the entry's index + 1 to be used as a unique ID.

So zero really is a special value, excellent.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:524-526
+      assert((Vars.empty() && EntryValuesBackupVars.empty()) ==
+                 VarLocs.empty() &&
+             "open ranges are inconsistent");
----------------
djtodoro wrote:
> jmorse wrote:
> > Won't having "Vars.empty()" as a clause here mean the assertion fires if the OpenRanges isn't empty, rather than testing whether it's empty?
> I am not sure any more, since this is not used at the current code..
I think it should probably read:

  assert(Vars.empty() == EmptyValuesBackupVars.empty &&
             Vars.Empty() == VarLocs.empty() &&
             "open ranges are inconsistent");
  return VarLocs.empty()

To ensure that if one is empty, all the others are. Simply asserting that Vars is empty will lead to assertions firing for completely valid states of the pass.


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

https://reviews.llvm.org/D68209