[PATCH] D31145: [Outliner] Fix compile-time overhead for candidate choice

Jessica Paquette via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 22 12:17:04 PDT 2017


paquette added inline comments.


================
Comment at: lib/CodeGen/MachineOutliner.cpp:580
   ///
-  /// \param N The suffix tree node to start pruning from.
-  /// \param Len The length of the string to be pruned.
+  /// FIXME: We only have to look at the leaves and their parents in the tree.
+  /// We should see if it's possible to save some space in the tree by taking
----------------
silvas wrote:
> silvas wrote:
> > This FIXME doesn't make sense. Every internal node is the parent (not just ancestor) of a leaf. In fact you are relying on this so that looking at parents of the leaves considers all internal nodes.
> > 
> Or the code is failing to consider all internal nodes with this patch, which seems like a bummer. Do you have data on how many internal nodes aren't parents of leaves (and have found the missed opportunities to be negligible)?
Yeah I was wrong here. I got too excited about the idea of getting rid of a bunch of nodes.


================
Comment at: lib/CodeGen/MachineOutliner.cpp:620
+      bool IsTailCall;
+      unsigned Benefit = BenefitFn(*Leaf, StringLen, IsTailCall);
 
----------------
silvas wrote:
> Should be `*Parent`, right? Leaves will always have 1 occurrence.
BenefitFn looks at the parent of the leaf, so it's fine. It should really be refactored to take in the parent though, since that's what the traversal is concerned with.


https://reviews.llvm.org/D31145





More information about the llvm-commits mailing list