[PATCH] D29668: Elide argument copies during instruction selection

[Reid Kleckner via Phabricator via llvm-commits]

rnk marked 6 inline comments as done.
rnk added a comment.

Thanks for the review!



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


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


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


================
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()));
----------------
MatzeB wrote:
> 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?
x86 at least says the opposite. If the ABI required that we not mutate the argument in memory, we wouldn't be able to do this transformation. So, maybe this is a good place to comment that the non-immutability is important.

Also, it looks like during the evolution of this patch, I ended up not needing `setIsImmutableObjectIndex`. I'll remove that. It's redundant with this object creation.


https://reviews.llvm.org/D29668