[PATCH] D104976: [AsmWriter] Properly handle uselistorder for global symbols

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

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

Thanks! This is a nice approach; LGTM with a couple of minor nits.

> Currently, AsmWriter will stick uselistorder directives for global values inside individual functions.

FWIW (not much I think), I vaguely remember a desire (mine? reviewers?) to avoid top-level `uselistorder` directives... but I guess at some point the dam broke. This is easier to reason about.



================
Comment at: llvm/lib/IR/AsmWriter.cpp:241
+    const Value *V = Pair.first;
+    if (!V->use_empty() && std::next(V->use_begin()) != V->use_end()) {
+      std::vector<unsigned> Shuffle =
----------------
Nit: this could be inverted with a `continue` to reduce nesting.


================
Comment at: llvm/lib/IR/AsmWriter.cpp:244
+          predictValueUseListOrder(V, Pair.second, OM);
+      if (!Shuffle.empty()) {
+        const Function *F = nullptr;
----------------
Nit: this could inverted with a `continue` to reduce nesting.


================
Comment at: llvm/test/Bitcode/use-list-order2.ll:2
 ; RUN: verify-uselistorder %s
-; XFAIL: *
 
----------------
Looks like this was disabled to "fix" PR24755, which was introduced by the refactor of Function operands... a bit unfortunate for the test to have been turned off instead of reverting the patch that broke it (this approach seems to defeat the point of adding tests?)...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104976