[PATCH] D29668: Elide argument copies during instruction selection

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

FWIW, this looks fine to me with minor tweaks below. Would be great to have @MatzeB be happy as well though, this is a part of the backend I'm somewhat less familiar with.



================
Comment at: include/llvm/Target/TargetCallingConv.h:48
     unsigned IsInConsecutiveRegs : 1;
+    unsigned IsCopyElisionCandidate : 1; ///< Argument copy elision candidate
 
----------------
rnk wrote:
> chandlerc wrote:
> > This isn't really that the argument is an elision candidate, it means the argument's copy is *elided* and you transform based on this.
> No, we don't. This flag tells the target that if the argument lives in memory, then it should return a FrameIndexSDNode, similar to the way that we would if the IsByVal flag was set. If the argument doesn't live in memory, it should ignore this flag. We only do the transform if a frame index comes back from `TLI->LowerFormalArguments`.
Ah, OK. The term "candidate" is confusing me I think, but I don't reall yhave a better idea for a name...


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8077
+      for (const Use &U : I.operands()) {
+        if (StaticAllocaInfo *Info = GetInfoIfStaticAlloca(U))
+          *Info = StaticAllocaInfo::Clobbered;
----------------
rnk wrote:
> chandlerc wrote:
> > For a sequence of N casts of an alloca, doesn't this do O(N^2) work?
> > 
> > Maybe make GetInfoIfStaticAlloca fully memoize each Value?
> Do you think the memory use of memoization is worth optimizing for long chains of casts that mid-level optimizations usually remove?
I just don't want a quadratic path. You could add a recursion limit instead if you never expect the tall chains in practice?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2733
+          MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI));
+    } else {
+      // This is not the first piece of an argument in memory. See if there is
----------------
Dy ou need the else? It's after a return...


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2747-2755
+      if (MFI.isFixedObjectIndex(FI)) {
+        SDValue Addr =
+            DAG.getNode(ISD::ADD, dl, PtrVT, DAG.getFrameIndex(FI, PtrVT),
+                        DAG.getIntPtrConstant(Ins[i].PartOffset, dl));
+        return DAG.getLoad(
+            ValVT, dl, Chain, Addr,
+            MachinePointerInfo::getFixedStack(DAG.getMachineFunction(), FI,
----------------
You could just put this inside the if above in the for loop...  Dunno, the loop above just feels awkward, but not sure there is really better way to do this.


https://reviews.llvm.org/D29668