[PATCH] D80684: [LiveDebugValues] Speed up removeEntryValue, NFC

[Vedant Kumar via Phabricator via llvm-commits]

vsk marked 5 inline comments as done.
vsk added a comment.

In D80684#2059689 <https://reviews.llvm.org/D80684#2059689>, @nikic wrote:

> Hm, for some reason I'm not seeing any instruction count differences with this patch (or at least the latest version), in either -O0 -g or -O3 -g configuration.


@nikic I can reproduce the problem using wall time alone. I did 5 CTMark runs at `{-O0 -g, -O3 -g} x {prepatch, postpatch}` (20 runs total, at -j1, no other stabilization), but found no significant geomean compile time change prepatch vs. postpatch. However, I do see a large speedup when compiling a separate copy of sqlite3.

It looks like CTMark ships the sqlite3 3.5.7 amalgamation (85 KLOC, generated in 2008), whereas I've been testing with the 3.30.1 amalgamation (225 KLOC, generated in 2019).

So, I think this change is worthwhile, but maybe it's not sufficient.



================
Comment at: llvm/include/llvm/ADT/CoalescingBitVector.h:363
+    if (StartIt == end() || *StartIt >= End)
+      return {end(), end()};
+    auto EndIt = StartIt;
----------------
aprantl wrote:
> It's weird that this empty range violates the assertion on line 360, but I think that is okay.
> 
> Is `*StartIt >= End` even possible, given the assertion?
Thanks for the careful review here. I've added simplified test cases that show why the `*StartIt >= End` check is necessary, and why we have to return an empty range in that case.

The situation is: say you have the bitvector `{1}`, and you ask for the half-open range `[0, 1)`. Keep in mind that find(X) returns a lower bound (the first bit set at, or after, X). So `*find(Start) = *find(0) = 1`, which means `*StartIt == End`. Since we want a half-open range, we don't want to include `1` in it, hence the empty range.

Similarly, if you have the bitvector `{5}` and you ask for the half-open range `[3, 4)`, `*find(Start) = *find(3) = 5`, which means `*StartIt > End` and we want to return the empty range again.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:136
+  /// kind RegisterKind.
+  static constexpr uint32_t kFirstInvalidRegLocation = 1 << 30;
+
----------------
aprantl wrote:
> what does the `k` stand for?
"Constant" -- it's somewhat idiomatic in that it's used a fair amount in llvm, but not consistently. I'm not attached to this. Happy to go with your preferred spelling.


================
Comment at: llvm/lib/CodeGen/LiveDebugValues.cpp:917
       const VarLoc &VL = VarLocIDs[LocIndex::fromRawInteger(ID)];
-      if (!VL.isEntryBackupLoc())
-        continue;
----------------
djtodoro wrote:
> I'm not sure we want to remove this line. The backup locations are `EntryValueBackupKind` and `EntryValueCopyBackupKind` only; the `EntryValueKind` is real location created from one of these two.
It looks like `VL.getEntryValueCopyBackupReg()` returns undef when VL.Kind isn't the right kind. So it seems safe to remove the `VL.isEntryBackupLoc()` check?


================
Comment at: llvm/unittests/ADT/CoalescingBitVectorTest.cpp:506
+    EXPECT_TRUE(rangesMatch(BV.half_open_range(2, 4), {2, 3}));
+    EXPECT_TRUE(rangesMatch(BV.half_open_range(4, 5), {}));
+  }
----------------
jmorse wrote:
> IMHO, we should also test a Start >= End case, as that's a condition in half_open_range.
Ah, are you referring to the same case Adrian described? If so, I've just added a test to cover that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80684