[PATCH] D141048: [SelectionDAG] Add pcsections recursively on SDNode values

[Marco Elver via Phabricator via llvm-commits]

melver added a comment.

Nice!

Minor comments below. Otherwise LGTM.



================
Comment at: llvm/include/llvm/CodeGen/SelectionDAG.h:2283
   void addPCSections(const SDNode *Node, MDNode *MD) {
-    SDEI[Node].PCSections = MD;
+    SmallPtrSet<const llvm::SDNode *, 32> Once{};
+    addPCSectionsr(Node, MD, Once);
----------------
This looks like DFS - this is typically called the visited set. So I'd just call it "Visited".


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAG.h:2363
+  /// Recursively set PCSections to be associated with Node and all its values.
+  void addPCSectionsr(const SDNode *Node, MDNode *MD,
+                      SmallPtrSet<const llvm::SDNode *, 32> Once) {
----------------
No need for different name, just "addPCSections" - the fact it has different arguments means it's overloaded and there's no conflict.

Given it's private and has documentation above it, it seems cleaner.


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAG.h:2363
+  /// Recursively set PCSections to be associated with Node and all its values.
+  void addPCSectionsr(const SDNode *Node, MDNode *MD,
+                      SmallPtrSet<const llvm::SDNode *, 32> Once) {
----------------
melver wrote:
> No need for different name, just "addPCSections" - the fact it has different arguments means it's overloaded and there's no conflict.
> 
> Given it's private and has documentation above it, it seems cleaner.
This is a rather large non-trivial function now, I'd now define it in the header, but move it to the .cpp file (like most functions here).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141048