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

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

This seems like it could be useful.  Do you have specific places in mind where you'd take advantage of it?



================
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();
+
----------------
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.


================
Comment at: unittests/IR/UseTest.cpp:114-118
+  const char *ModuleString = "define void @f(i32 %x) {\n"
+                             "entry:\n"
+                             "  %v0 = inttoptr i32 %x to i8*\n"
+                             "  ret void\n"
+                             "}\n";
----------------
This might be simpler to specify with a raw string literal.  You could also make this large enough to be interesting and specify the use-list, via `uselistorder` directives (see: http://llvm.org/docs/LangRef.html#use-list-order-directives).



================
Comment at: unittests/IR/UseTest.cpp:131
+  Instruction *Ret = F->getEntryBlock().getTerminator();
+  constexpr unsigned Size = 20 * 1000;
+  for (unsigned I = 1; I <= Size; ++I) {
----------------
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?


================
Comment at: unittests/IR/UseTest.cpp:134
+    auto *Const = ConstantInt::get(X.getType(), I);
+    BinaryOperator::Create(Instruction::Add, &X, Const, "v" + Twine(I), Ret);
+  }
----------------
Do you really need to generate IR here?


================
Comment at: unittests/IR/UseTest.cpp:152
+  constexpr unsigned NumRepetitions = 100;
+  for (unsigned J = 0; J < NumRepetitions; ++J)
+    for (Instruction &I : F->getEntryBlock())
----------------
Why do you have so many repetitions?


================
Comment at: unittests/IR/UseTest.cpp:163
+        }
+  ASSERT_EQ(Size * NumRepetitions, FoundCounter);
+  ASSERT_EQ(Size * NumRepetitions + Size, CheckCounter);
----------------
You've run a lot of tests, and are only checking the result at the end.  It will be hard to debug if this every goes wrong.  What's the key thing you're trying to test here?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58098