[PATCH] D29668: Elide argument copies during instruction selection

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: include/llvm/CodeGen/SelectionDAGISel.h:318
+
+  void findArgumentCopyElisionCandidates();
+
----------------
no doxygen?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8047
+  enum StaticAllocaInfo { Unknown, Clobbered, Elidable };
+  DenseMap<const AllocaInst *, StaticAllocaInfo> StaticAllocas;
+
----------------
SmallDenseMap? Seems like we can predict common upper bounds and this is a leaf-y function, so can avoid most memory allocation here.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8077
+      for (const Use &U : I.operands()) {
+        if (StaticAllocaInfo *Info = GetInfoIfStaticAlloca(U))
+          *Info = StaticAllocaInfo::Clobbered;
----------------
For a sequence of N casts of an alloca, doesn't this do O(N^2) work?

Maybe make GetInfoIfStaticAlloca fully memoize each Value?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8168-8171
+  // Look for stores of arguments to static allocas. Mark such arguments with a
+  // flag to ask the target to give us the memory location of that argument if
+  // available.
+  findArgumentCopyElisionCandidates();
----------------
Both `findArgumentCopyElisionCandidates` and `elideArgumentCopy` are only ever called from this function. Maybe make them local static helpers instead of putting them into the API?

Also, the only data structure used outside this function is the `ElidedArgCopyInstrs`. Maybe sink all the other data structtures down to be local? That seems like it would also help address Hans' comments about name.

I also don't see why you need flags for this when you have a data structure that tracks which things are candidates...


https://reviews.llvm.org/D29668