[PATCH] D142620: [Coroutines] Improve rematerialization stage

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu requested changes to this revision.
ChuanqiXu added a comment.
This revision now requires changes to proceed.

Thanks for working on this! But a problem I found is that it is expensive to construct `ReversePostOrderTraversal` and this patch tries to construct it in a loop. So it looks not so good to me.

And I am wondering if it is necessary to have such a complex structure and algorithm. What I had  in mind is that we can use a worklist to store the materialized instructions and we can operate on that list. So we can avoid duplicate and meaningless iterations. I feel this is easier to implement and it looks not bad.



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:326
+  Instruction *Node;
+  SmallVector<RematNode *> Children;
+  RematNode() = default;
----------------
What does `Children` mean here?


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:387-388
+
+  inline RematNode **child_begin(RematNode *N) { return N->Children.begin(); }
+  inline RematNode **child_end(RematNode *N) { return N->Children.end(); }
+
----------------
Are the 2 methods used?


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2225-2226
 
 // For every use of the value that is across suspend point, recreate that value
 // after a suspend point.
+static void rewriteMaterializableInstructions(
----------------
The comment looks not precise after we land this patch.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2248
+    RematGraph *RG = E.second.get();
+    ReversePostOrderTraversal<RematGraph *> RPOT(RG);
+    using rpo_iterator = ReversePostOrderTraversal<RematGraph *>::rpo_iterator;
----------------
It is expensive to create ReversePostOrderTraversal. So it looks not good to construct it in a loop.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2249
+    ReversePostOrderTraversal<RematGraph *> RPOT(RG);
+    using rpo_iterator = ReversePostOrderTraversal<RematGraph *>::rpo_iterator;
+
----------------
I feel it is not so necessary and helpful to declare the type for the iterator.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2901-2907
+      // Manually add dbg.value metadata uses of I.
+      SmallVector<DbgValueInst *, 16> DVIs;
+      findDbgValues(DVIs, &I);
+      for (auto *DVI : DVIs)
+        if (Checker.isDefinitionAcrossSuspend(I, DVI))
+          Spills[&I].push_back(DVI);
+    }
----------------
This is not good. It may cause the the behavior become inconsistent after we materialize DVI instructions. See https://github.com/llvm/llvm-project/issues/55276 for an example.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2928
+    for (Instruction *U : E.second) {
+      // Don't process a use twice (this can happen if the instruction uses
+      // more than one rematerializable def)
----------------



================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2930-2933
+      if (!AllRemats.count(U)) {
+        // Constructor creates the whole RematGraph for the given Use
+        std::unique_ptr<RematGraph> RematUPtr =
+            std::make_unique<RematGraph>(U, Checker);
----------------
We may prefer such styles to shorten the indentation.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2932-2933
+        // Constructor creates the whole RematGraph for the given Use
+        std::unique_ptr<RematGraph> RematUPtr =
+            std::make_unique<RematGraph>(U, Checker);
+
----------------
nit: We can use `auto` if we can see the type in the right hand side.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2938-2940
+            using rpo_iterator =
+                ReversePostOrderTraversal<RematGraph *>::rpo_iterator;
+            for (rpo_iterator I = RPOT.begin(); I != RPOT.end();
----------------
It looks not bad to use `auto` in this case.


================
Comment at: llvm/lib/Transforms/Coroutines/CoroFrame.cpp:2952-2955
+  IRBuilder<> Builder(F.getContext());
+  rewriteMaterializableInstructions(Builder, AllRemats);
+
+  Spills.clear();
----------------
We can construct the IRBuilder in rewriteMaterializableInstructions and we don't need to clear the Spills clearly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142620