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

[Ehud Katz via Phabricator via llvm-commits]

ekatz marked 6 inline comments as done.
ekatz added a comment.

Thanks for the review! I really appreciate it, as I know this is a complicated one.
I'll add more documentation, and try to make it simpler/clearer.



================
Comment at: llvm/include/llvm/ADT/Waymarking.h:41
+//         '---'---'---'---'---'---'---'---'---'---'---'---'---'---'---'---'
+//             |-1 |-2     |-4         |-7         |-10                |-14
+//          <_ |   |       |           |           |                   |
----------------
foad wrote:
> Is the rightmost vertical line one bit too far to the right here?
Correct. I'll fix it.


================
Comment at: llvm/include/llvm/ADT/Waymarking.h:119
+
+template <unsigned NumBits> struct WaymarkingTraits {
+  enum : unsigned {
----------------
rriddle wrote:
> 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.
Sure, I will.


================
Comment at: llvm/include/llvm/ADT/Waymarking.h:212
+void fillWaymarks(TIter Begin, TIter End, size_t Offset = 0) {
+  if (!(Begin < End))
+    return;
----------------
rriddle wrote:
> 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?
You are correct. The more general case is to use the `==` and `!=` operators. I'll change that.
Regarding `llvm::Use`, it has an inherent solution, using a //reverse iterator// on the use-array.


================
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) {
----------------
rriddle wrote:
> Can you please add more comments on the specific things going on here?
Will do.


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


================
Comment at: llvm/unittests/ADT/WaymarkingTest.cpp:18
+
+int tag(int *P) {
+  return static_cast<int>(reinterpret_cast<uintptr_t>(P) &
----------------
rriddle wrote:
> 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).
Will do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74415