[PATCH] D58098: [IR] Add Use::moveToFrontOfUseList() method

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin marked 2 inline comments as done.
rtereshin added inline comments.


================
Comment at: include/llvm/IR/Use.h:132
+  /// close to it) of the use list, reducing the amortized cost of such lookups.
+  inline void moveToFrontOfUseList();
+
----------------
dexonsmith wrote:
> rtereshin wrote:
> > dexonsmith wrote:
> > > I'm curious if there's really a compelling reason to define this inline in Value.h.  It doesn't seem like it will be used very frequently (and I assume most vendors compile with some form of LTO anyway), so without profiling data I'd rather this just be in Use.cpp.
> > Ah, I missed that one.
> > 
> > I didn't profile anything with LTO on, but with a regular -O3 build the in Release the unittest included here performed ever so slightly better (with larger repetition counts), I don't quite remember the exact numbers already, but I think it was a couple of percent.
> > 
> > Also, `Use::set` is defined like this and I liked the idea of putting them next to each other because they are very similar in implementation. `moveToFrontOfUseList` is practically a specialization of `set` for the current value of the use.
> > 
> > If that doesn't change your mind, I'll put it in cpp, I don't have a strong opinion about this.
> If you're seeing real speedups on a full compilation pipeline for a real workload (or for running `check-llvm`), then it might be worth it, but otherwise I'd rather put it in the cpp and trust vendors who care about performance to use LTO.  Maybe `set` should move as well but it's widely-used enough that it could slow down `check-llvm` (although I bet no one has measured recently).
Okay, I'll move it into cpp.


================
Comment at: unittests/IR/UseTest.cpp:131
+  Instruction *Ret = F->getEntryBlock().getTerminator();
+  constexpr unsigned Size = 20 * 1000;
+  for (unsigned I = 1; I <= Size; ++I) {
----------------
dexonsmith wrote:
> rtereshin wrote:
> > dexonsmith wrote:
> > > This seems really big.  What extra coverage are you getting from this?
> > > 
> > > Generally, there seems to be a lot of permutation here that is unrelated to the new API.  It'll take time to run.  What value is it adding?
> > For some time I had that idea that it could be useful if we had "compile time regression tests", which are also reliable and fast. I think it's possible when a speed up achieved is asymptotically significant, like going from O(n^2) algorithm to O(n) one. If the n is big enough and the constant factor is small enough, it may be possible to design a test which is very fast even when ran on very slow machines (bots or whatnot) if the compile time problem is still in check, but really, really slow even on fastest machines if there is a regression.
> > 
> > This test takes about 50 ms to run if `moveToFrontOfUseList` does what's expected, as well as the other operations (like adding a new use) behave like they currently do (inserting new uses in the beginning of the list, for instance). If any of it breaks apart, this test takes almost 2 minutes to run (before it fails with one of the google-tests assertions that check the values of the "performance counters", which in this case specifically are possible).
> > 
> > Please let me know what do you think!
> > For some time I had that idea that it could be useful if we had "compile time regression tests", which are also reliable and fast. 
> 
> Seems like a great idea to have, but it probably deserves a broader discussion on llvm-dev about how they should be integrated into the test suite, whether they're part of `check-llvm` or a separate `check-llvm-perf`, etc.; personally I'm a bit skeptical of having them intermingled with non-performance tests without having them stand out *somehow*.
> 
> Maybe you've had a discussion already and I missed it?  If so, or if there's significant precedent of this already in tree, please point me at it so I can catch up :).  But even in that case, I think you should have an independent test that checks the functionality, separately from a test that checks the performance of this high-level task.
> Maybe you've had a discussion already and I missed it? If so, or if there's significant precedent of this already in tree, please point me at it so I can catch up :).

If there is a precedent, I'm not aware of it. I'll change the test so it would only perform so many actions to catch the trend, no more, and add a purely functional one.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58098