[PATCH] D29668: Elide argument copies during instruction selection

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 2 inline comments as done.
rnk added inline comments.


================
Comment at: include/llvm/Target/TargetCallingConv.h:48
     unsigned IsInConsecutiveRegs : 1;
+    unsigned IsCopyElisionCandidate : 1; ///< Argument copy elision candidate
 
----------------
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`.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8122
+    if (!Arg || Arg->hasInAllocaAttr() || Arg->hasByValAttr() ||
+        Arg->getType()->isEmptyTy() ||
+        DL.getTypeStoreSize(Arg->getType()) !=
----------------
majnemer wrote:
> Would `!Arg->getType()->isSized()` be more precise?
That ignores element counts on arrays and vectors, right? Do you think that's right? I think what I really wanted to say was "I don't know what the heck would happen if we elided the copy of a zero sized argument, so let's not try".


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8132-8134
+    // Mark this alloca and store for argument copy elision.
+    *Info = StaticAllocaInfo::Elidable;
+    ArgCopyElisionCandidates.insert({Arg, {AI, SI}});
----------------
chandlerc wrote:
> Would it be worth stopping the scan when you're out of arguments?
Yeah, probably for -O0 builds with tons of allocas.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8278-8279
       Flags.setOrigAlign(OriginalAlignment);
+      if (ArgCopyElisionCandidates.count(&Arg))
+        Flags.setCopyElisionCandidate();
 
----------------
chandlerc wrote:
> You erase things from the set below but don't clear the flags.... I assume this can't manifest because of how the flag is currently used, but it seems surprising at the least. Maybe sink this to the below code where you're going to do the elision?
The flags are just temporary data, and they're only supposed to indicate candidates, not successfully transformed arguments. It doesn't seem worth clearing them.

I did notice that I can check the flag instead of doing a hash lookup below, though.


https://reviews.llvm.org/D29668