[PATCH] D29668: Elide argument copies during instruction selection

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.h:88
     assert((!E || E->isValid()) && "Expected valid expression");
-    assert(~FI && "Expected valid index");
+    assert(FI != INT_MAX && "Expected valid index");
 
----------------
hans wrote:
> Unrelated, just commit?
It's related because this change triggers the assertion. As written, the assert is equivalent to:
  assert(FI != -1);
However, -1 is a valid fixed frame index, and it is typically the one used for the first argument in memory.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8057
+    auto Iter = StaticAllocas.insert({AI, Unknown});
+    return &Iter.first->second;
+  };
----------------
hans wrote:
> I might have gone with just `return &StaticAllocas[AI]`
This is C++! We can't do two hash lookups. :)


================
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();
----------------
chandlerc wrote:
> 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...
I had it that way originally, but the set of elided instructions needs to make it into SelectBasicBlock so we can skip over them. At that point I made these things instance methods and fields. I'll flip it around and pass the data explicitly and see if we like that better.


================
Comment at: test/CodeGen/X86/arg-copy-elide.ll:19
+; CHECK: pushl %[[reg]]
+; CHECK: calll _addrof_i32
+; CHECK: retl
----------------
hans wrote:
> Is there a pop or something restoring the stack after the call? Should we check for that too?
There is, but I don't want to overfit our choice for how to adjust the stack. It could be "pop" or "add $4, %esp". Matching the push doesn't feel like overfitting because it's a profitable transform that we're likely to keep.


https://reviews.llvm.org/D29668