[PATCH] D43036: Use a stable topological sort instead of std::stable_sort in DIE *DwarfCompileUnit::createScopeChildrenDIE()

[Volodymyr Sapsai via Phabricator via llvm-commits]

vsapsai added a comment.

Sorting algorithm looks correct to me and in debugger it works as expected. I don't think we are going to visit `DbgVariable` more than 3 times so time complexity is reasonable too.

There is a problem with `SmallDenseSet` size and other than that maybe improve testing? Some of the ideas: make variables in order "array, unrelated, vla_expr" to expose problem with std::stable_sort; have dependencies that aren't local variables (this can be already covered by other tests); have more than 8 local variables that is not a power of 2.



================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:589
 /// expressions come first.
-static bool sortLocalVars(DbgVariable *A, DbgVariable *B) {
-  return dependsOn(B, A);
+static SmallVector<DbgVariable *, 8> sortLocalVars(SmallVectorImpl<DbgVariable *> &Input) {
+  SmallVector<DbgVariable *, 8> Result;
----------------
Parameter can be const and the line is > 80 characters.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:595
+  /// Set of DbgVariables in Result.
+  SmallDenseSet<DbgVariable *, 8> Visited(Input.size());
+  /// For cycle detection.
----------------
It worked on the reduced test case but for a bigger file it failed with
> Assertion failed: ((getNumBuckets() & (getNumBuckets()-1)) == 0 && "# initial buckets must be a power of two!"), function initEmpty, file llvm/include/llvm/ADT/DenseMap.h, line 344.

That happened for the `Input` of size 10.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:596
+  SmallDenseSet<DbgVariable *, 8> Visited(Input.size());
+  /// For cycle detection.
+  SmallDenseSet<DbgVariable *, 8> Visiting(Input.size());
----------------
You have a few comments inside the function that start with triple slash. Is this on purpose? Not saying it is wrong, but I would use just 2 slashes inside the function, so maybe it is a chance for me to learn something new.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:605
+    auto Item = WorkList.back();
+    DbgVariable *N = Item.getPointer();
+    bool visitedAllDependencies = Item.getInt();
----------------
I am unfamiliar with this code so I can be completely wrong. For me, subjectively, `N` strongly indicates it is a number but in this case it is not. So I need to keep in mind that `N` is of type `DbgVariable`. It get's especially confusing when it is next to actual number like `{N, 1}`. To avoid criticizing without offering alternatives, personally, I would prefer `Var` and later for dependencies maybe to use `LocalVar`.

It is a personal readability opinion, it's up to you to decide how well it actually works.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp:637
+      auto Var = dyn_cast_or_null<const DILocalVariable>(Dependency);
+      WorkList.push_back({DbgVar[Var], 0});
+    }
----------------
What would happen and if it is even possible if `Dependency` is not a local variable?

And in this case `dyn_cast<>` should be enough, we don't add nullptr dependencies.


https://reviews.llvm.org/D43036