[PATCH] D74415: [ADT] Implement the Waymarking as an independent utility

[River Riddle via Phabricator via llvm-commits]

rriddle added a comment.

Looks mostly good to me. I think the main thing is that more documentation would be nice, waymarking can be extremely hard to follow at times if you aren't familiar with it. This is why it took me a bit to review, I had to make sure that I could follow all of the different templates correctly. That being said, I'm not sure that I am comfortable enough doing the final approval here.



================
Comment at: llvm/include/llvm/ADT/Waymarking.h:119
+
+template <unsigned NumBits> struct WaymarkingTraits {
+  enum : unsigned {
----------------
Can you add better documentation for this? I know this is an implementation detail, but it would help those of us that don't really know waymarking that well.


================
Comment at: llvm/include/llvm/ADT/Waymarking.h:212
+void fillWaymarks(TIter Begin, TIter End, size_t Offset = 0) {
+  if (!(Begin < End))
+    return;
----------------
Why use `<` instead of `==` or `!=`? This seems to apply a specific iterator pattern. For example, I don't think this would work for `llvm::Use` which may be allocated before the User. In that case the tags are assigned in reverse, with the end of the array being assigned the full stop tag. Could you add support and a test case for this potential usage?


================
Comment at: llvm/include/llvm/ADT/Waymarking.h:216
+  size_t Count;
+  if (Offset <= Marker::Traits::NUM_STATIC_TAGS) {
+    while (Offset != Marker::Traits::NUM_STATIC_TAGS) {
----------------
Can you please add more comments on the specific things going on here?


================
Comment at: llvm/include/llvm/ADT/Waymarking.h:227
+
+    Count = Marker::Traits::Tags::Remain;
+  } else {
----------------
Seems like Count is assigned to the same in both branches, can we de-duplicate?


================
Comment at: llvm/unittests/ADT/WaymarkingTest.cpp:18
+
+int tag(int *P) {
+  return static_cast<int>(reinterpret_cast<uintptr_t>(P) &
----------------
nit: Can you please hoist all of these static functions into the global namespace and mark them with 'static'? LLVM style is to generally only use anonymous namespaces for classes(or in this case, TEST).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74415