[PATCH] D29668: Elide argument copies during instruction selection

[Hans Wennborg via Phabricator via llvm-commits]

hans added a comment.

Looks good as far as I can tell.

It adds more complexity to SelectionDAG, but I suppose it's the best place to do it.



================
Comment at: include/llvm/CodeGen/SelectionDAGISel.h:59
+      CopyElisionCandidates;
+  SmallPtrSet<const Instruction *, 4> ElidedInstructions;
+  DenseMap<int, int> ElisionFrameIndexMap;
----------------
These are pretty generalist names in a broad scope. Maybe name them around "ArgCopy" rather than just Copy or Elided instructions?

I suppose for the consumer of this set it doesn't care what kind of instruction it is really.. bot for ElisionFrameIndexMap at least maybe ArgCpyElisionFrameIndexMap would be better? (Pretty long though..)


================
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");
 
----------------
Unrelated, just commit?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8057
+    auto Iter = StaticAllocas.insert({AI, Unknown});
+    return &Iter.first->second;
+  };
----------------
I might have gone with just `return &StaticAllocas[AI]`


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8463
   EmitFunctionEntryCode();
+  CopyElisionCandidates.clear();
 }
----------------
You cleared it above too before starting to fill it, so maybe one of these is redundant?


================
Comment at: test/CodeGen/X86/arg-copy-elide.ll:19
+; CHECK: pushl %[[reg]]
+; CHECK: calll _addrof_i32
+; CHECK: retl
----------------
Is there a pop or something restoring the stack after the call? Should we check for that too?


https://reviews.llvm.org/D29668