[PATCH] D29668: Elide argument copies during instruction selection

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added inline comments.


================
Comment at: include/llvm/CodeGen/MachineFrameInfo.h:573
 
+  /// setIsImmutableObjectIndex - Marks the immutability of an object.
+  void setIsImmutableObjectIndex(int ObjectIdx, bool Immutable) {
----------------
Do not repeat the name of the documented function.


================
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;
----------------
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.


================
Comment at: include/llvm/Target/TargetCallingConv.h:128-129
 
+    bool isCopyElisionCandidate()  const { return Flags & InConsecutiveRegsLast; }
+    void setCopyElisionCandidate() { Flags |= One << InConsecutiveRegsLastOffs; }
+
----------------
Wrong Copy&Paste!


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8051
+findArgumentCopyElisionCandidates(const DataLayout &DL,
+                                  FunctionLoweringInfo *FuncInfo,
+                                  ArgCopyElisionMapTy &ArgCopyElisionCandidates) {
----------------
`FuncInfo` could be a reference as it mustn't be nullptr.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8074
+  // GEPs to handle type coercions, as long as the alloca is fully initialized
+  // by the store.
+  // FIXME: Handle split stores.
----------------
Maybe document that the address of those allocas must not escape until a store is found (because that explains some of the complexity in the loop).


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8077-8082
+    // We will look through cast uses, so ignore them completely.
+    if (I.isCast())
+      continue;
+    // Ignore debug info intrinsics, they don't escape or store to allocas.
+    if (isa<DbgInfoIntrinsic>(I))
+      continue;
----------------
You could move those two checks into the `if (!SI)` block.


================
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) {
----------------
Most arguments could be reference as they mustn't be nullptr.


================
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));
----------------
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.




================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8462
+  if (!Chains.empty())
+    NewRoot =  DAG.getNode(ISD::TokenFactor, dl, MVT::Other, Chains);
+
----------------
extra space after `=`


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2718
+    int FI = MFI.CreateFixedObject(ValVT.getSizeInBits() / 8,
+                                   VA.getLocMemOffset(), false);
+    return DAG.getFrameIndex(FI, getPointerTy(DAG.getDataLayout()));
----------------
I don't know right now what the ABIs say, but I would expect the argument memory to be immutable. So maybe something like `!AlwayseUseMutable` would work here?


https://reviews.llvm.org/D29668