[PATCH] D29668: Elide argument copies during instruction selection

[Matthias Braun via Phabricator via llvm-commits]

MatzeB accepted this revision.
MatzeB added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: include/llvm/Target/TargetCallingConv.h:71-72
+    static const uint64_t ElideCopyOffs  = 61;
+    static const uint64_t InConsecutiveRegsLast      = 0x1ULL<<62;
     static const uint64_t InConsecutiveRegsLastOffs  = 62;
+    static const uint64_t InConsecutiveRegs      = 0x1ULL<<63;
----------------
rnk wrote:
> MatzeB wrote:
> > This header makes me cry: Define everything twice as bit and as the number of the bit. Just to use the number of the bit in a `One << XXX` expression below... Anyway unrelated to this patch which stays consistent with the existing madness.
> Given my copy-pasto, I fixed this in rL294989. I can't believe how long we've lived with this manual masking.
Thanks, this new version is so much simpler!


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8131-8135
+static bool elideArgumentCopy(FunctionLoweringInfo *FuncInfo,
+                              DenseMap<int, int> &ArgCopyElisionFrameIndexMap,
+                              SmallPtrSetImpl<const Instruction *> &ElidedArgCopyInstrs,
+                              const Argument *Arg, const AllocaInst *AI,
+                              int FixedIndex, const StoreInst *SI) {
----------------
rnk wrote:
> MatzeB wrote:
> > Most arguments could be reference as they mustn't be nullptr.
> Many of these are used in pointer-y ways like being inserted into a pointer set. IMO they're better as pointers.
It's certainly a subjective thing given that much of the existing code is written with pointers. I don't have a strong opinion on it, so keep it.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8402
+          ArgValues.push_back(
+              DAG.getLoad(VT, dl, NewRoot, InVals[i], MachinePointerInfo()));
+          Chains.push_back(ArgValues.back().getValue(1));
----------------
rnk wrote:
> MatzeB wrote:
> > Intuitively I would assume this should get a MO_Invariant flag for pretty much all ABIs here. I think however there is other code deducing this from the frame index, so it is probably fine to use the default pointer info here. If you haven't yet, please test that when you build an optimized function with/without your patch you get the same memory operands.
> > 
> > 
> Actually, we need to ensure that the load is not invariant. Once we elide the copy, we can end up mutating the argument in the caller allocated memory. I have a test for this.
Interesting, I didn't know that is legal. Fine with me then.


https://reviews.llvm.org/D29668