Skip to content

Commit

Permalink
[SDAGBuilder] Handle multi-part arguments in argument copy elision (P…
Browse files Browse the repository at this point in the history
…R63430)

When eliding an argument copy, we need to update the chain to ensure
the argument reads are performed before later writes. However, the
code doing this only handled this for the first part of the argument.
If the argument had multiple parts, the chains of the later parts were
dropped. Make sure we preserve all chains.

Fixes #63430.
  • Loading branch information
nikic committed Jun 22, 2023
1 parent d179421 commit 81ec494
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 6 deletions.
14 changes: 10 additions & 4 deletions llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10616,9 +10616,9 @@ static void tryToElideArgumentCopy(
DenseMap<int, int> &ArgCopyElisionFrameIndexMap,
SmallPtrSetImpl<const Instruction *> &ElidedArgCopyInstrs,
ArgCopyElisionMapTy &ArgCopyElisionCandidates, const Argument &Arg,
SDValue ArgVal, bool &ArgHasUses) {
ArrayRef<SDValue> ArgVals, bool &ArgHasUses) {
// Check if this is a load from a fixed stack object.
auto *LNode = dyn_cast<LoadSDNode>(ArgVal);
auto *LNode = dyn_cast<LoadSDNode>(ArgVals[0]);
if (!LNode)
return;
auto *FINode = dyn_cast<FrameIndexSDNode>(LNode->getBasePtr().getNode());
Expand Down Expand Up @@ -10661,7 +10661,8 @@ static void tryToElideArgumentCopy(
MFI.setIsImmutableObjectIndex(FixedIndex, false);
AllocaIndex = FixedIndex;
ArgCopyElisionFrameIndexMap.insert({OldIndex, FixedIndex});
Chains.push_back(ArgVal.getValue(1));
for (SDValue ArgVal : ArgVals)
Chains.push_back(ArgVal.getValue(1));

// Avoid emitting code for the store implementing the copy.
const StoreInst *SI = ArgCopyIter->second.second;
Expand Down Expand Up @@ -10922,9 +10923,14 @@ void SelectionDAGISel::LowerArguments(const Function &F) {
// Elide the copying store if the target loaded this argument from a
// suitable fixed stack object.
if (Ins[i].Flags.isCopyElisionCandidate()) {
unsigned NumParts = 0;
for (EVT VT : ValueVTs)
NumParts += TLI->getNumRegistersForCallingConv(*CurDAG->getContext(),
F.getCallingConv(), VT);

tryToElideArgumentCopy(*FuncInfo, Chains, ArgCopyElisionFrameIndexMap,
ElidedArgCopyInstrs, ArgCopyElisionCandidates, Arg,
InVals[i], ArgHasUses);
ArrayRef(&InVals[i], NumParts), ArgHasUses);
}

// If this argument is unused then remember its value. It is used to generate
Expand Down
4 changes: 2 additions & 2 deletions llvm/test/CodeGen/X86/pr63430.ll
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --no_x86_scrub_sp --version 2
; RUN: llc -mtriple=x86_64-unknown-linux < %s | FileCheck %s

; TODO: This is a miscompile.
; Make sure the argument is read before the stack slot is over-written.
define i1 @test(ptr %a0, ptr %a1, ptr %a2, ptr %a3, ptr %a4, ptr %a5, i128 %x) {
; CHECK-LABEL: test:
; CHECK: # %bb.0:
; CHECK-NEXT: movq 8(%rsp), %rax
; CHECK-NEXT: xorps %xmm0, %xmm0
; CHECK-NEXT: movaps %xmm0, 8(%rsp)
; CHECK-NEXT: andq 16(%rsp), %rax
; CHECK-NEXT: movaps %xmm0, 8(%rsp)
; CHECK-NEXT: cmpq $-1, %rax
; CHECK-NEXT: sete %al
; CHECK-NEXT: retq
Expand Down

0 comments on commit 81ec494

Please sign in to comment.