[llvm] r236916 - ScheduleDAGInstrs: In functions with tail calls PseudoSourceValues are not non-aliasing distinct objects
Quentin Colombet
qcolombet at apple.com
Fri May 8 17:11:58 PDT 2015
Hi Arnold,
The PowerPC part of the patch does not compile.
/lib/Target/PowerPC/PPCISelLowering.cpp:4218:5: error: use of undeclared identifier 'MF'
MF.getFrameInfo()->setHasTailCall();
^
1 error generated.
Could you fix or revert please?
Cheers,
-Quentin
> On May 8, 2015, at 4:52 PM, Arnold Schwaighofer <aschwaighofer at apple.com> wrote:
>
> Author: arnolds
> Date: Fri May 8 18:52:00 2015
> New Revision: 236916
>
> URL: http://llvm.org/viewvc/llvm-project?rev=236916&view=rev
> Log:
> ScheduleDAGInstrs: In functions with tail calls PseudoSourceValues are not non-aliasing distinct objects
>
> The code that builds the dependence graph assumes that two PseudoSourceValues
> don't alias. In a tail calling function two FixedStackObjects might refer to the
> same location. Worse 'immutable' fixed stack objects like function arguments are
> not immutable and will be clobbered.
>
> Change this so that a load from a FixedStackObject is not invariant in a tail
> calling function and don't return a PseudoSourceValue for an instruction in tail
> calling functions when building the dependence graph so that we handle function
> arguments conservatively.
>
> Fix for PR23459.
>
> rdar://20740035
>
> Added:
> llvm/trunk/test/CodeGen/AArch64/tailcall_misched_graph.ll
> Modified:
> llvm/trunk/include/llvm/CodeGen/MachineFrameInfo.h
> llvm/trunk/lib/CodeGen/ScheduleDAGInstrs.cpp
> llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp
> llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp
> llvm/trunk/lib/Target/Hexagon/HexagonISelLowering.cpp
> llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp
> llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
> llvm/trunk/test/CodeGen/ARM/debug-frame.ll
> llvm/trunk/test/CodeGen/ARM/ehabi.ll
> llvm/trunk/test/CodeGen/X86/tailcallstack64.ll
>
> Modified: llvm/trunk/include/llvm/CodeGen/MachineFrameInfo.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/MachineFrameInfo.h?rev=236916&r1=236915&r2=236916&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/CodeGen/MachineFrameInfo.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/MachineFrameInfo.h Fri May 8 18:52:00 2015
> @@ -246,6 +246,11 @@ class MachineFrameInfo {
> /// True if this is a varargs function that contains a musttail call.
> bool HasMustTailInVarArgFunc;
>
> + /// True if this function contains a tail call. If so immutable objects like
> + /// function arguments are no longer so. A tail call *can* override fixed
> + /// stack objects like arguments so we can't treat them as immutable.
> + bool HasTailCall;
> +
> /// Not null, if shrink-wrapping found a better place for the prologue.
> MachineBasicBlock *Save;
> /// Not null, if shrink-wrapping found a better place for the epilogue.
> @@ -281,6 +286,7 @@ public:
> HasMustTailInVarArgFunc = false;
> Save = nullptr;
> Restore = nullptr;
> + HasTailCall = false;
> }
>
> /// hasStackObjects - Return true if there are any stack objects in this
> @@ -508,6 +514,10 @@ public:
> bool hasMustTailInVarArgFunc() const { return HasMustTailInVarArgFunc; }
> void setHasMustTailInVarArgFunc(bool B) { HasMustTailInVarArgFunc = B; }
>
> + /// Returns true if the function contains a tail call.
> + bool hasTailCall() const { return HasTailCall; }
> + void setHasTailCall() { HasTailCall = true; }
> +
> /// getMaxCallFrameSize - Return the maximum size of a call frame that must be
> /// allocated for an outgoing function call. This is only available if
> /// CallFrameSetup/Destroy pseudo instructions are used by the target, and
> @@ -545,6 +555,9 @@ public:
> /// isImmutableObjectIndex - Returns true if the specified index corresponds
> /// to an immutable object.
> bool isImmutableObjectIndex(int ObjectIdx) const {
> + // Tail calling functions can clobber their function arguments.
> + if (HasTailCall)
> + return false;
> assert(unsigned(ObjectIdx+NumFixedObjects) < Objects.size() &&
> "Invalid Object Idx!");
> return Objects[ObjectIdx+NumFixedObjects].isImmutable;
>
> Modified: llvm/trunk/lib/CodeGen/ScheduleDAGInstrs.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/ScheduleDAGInstrs.cpp?rev=236916&r1=236915&r2=236916&view=diff
> ==============================================================================
> --- llvm/trunk/lib/CodeGen/ScheduleDAGInstrs.cpp (original)
> +++ llvm/trunk/lib/CodeGen/ScheduleDAGInstrs.cpp Fri May 8 18:52:00 2015
> @@ -20,6 +20,7 @@
> #include "llvm/Analysis/ValueTracking.h"
> #include "llvm/CodeGen/LiveIntervalAnalysis.h"
> #include "llvm/CodeGen/MachineFunctionPass.h"
> +#include "llvm/CodeGen/MachineFrameInfo.h"
> #include "llvm/CodeGen/MachineInstrBuilder.h"
> #include "llvm/CodeGen/MachineMemOperand.h"
> #include "llvm/CodeGen/MachineRegisterInfo.h"
> @@ -143,6 +144,13 @@ static void getUnderlyingObjectsForInstr
>
> if (const PseudoSourceValue *PSV =
> (*MI->memoperands_begin())->getPseudoValue()) {
> + // Function that contain tail calls don't have unique PseudoSourceValue
> + // objects. Two PseudoSourceValues might refer to the same or overlapping
> + // locations. The client code calling this function assumes this is not the
> + // case. So return a conservative answer of no known object.
> + if (MFI->hasTailCall())
> + return;
> +
> // For now, ignore PseudoSourceValues which may alias LLVM IR values
> // because the code that uses this function has no way to cope with
> // such aliases.
>
> Modified: llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp?rev=236916&r1=236915&r2=236916&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp (original)
> +++ llvm/trunk/lib/Target/AArch64/AArch64ISelLowering.cpp Fri May 8 18:52:00 2015
> @@ -2873,8 +2873,10 @@ AArch64TargetLowering::LowerCall(CallLow
>
> // If we're doing a tall call, use a TC_RETURN here rather than an
> // actual call instruction.
> - if (IsTailCall)
> + if (IsTailCall) {
> + MF.getFrameInfo()->setHasTailCall();
> return DAG.getNode(AArch64ISD::TC_RETURN, DL, NodeTys, Ops);
> + }
>
> // Returns a chain and a flag for retval copy to use.
> Chain = DAG.getNode(AArch64ISD::CALL, DL, NodeTys, Ops);
>
> Modified: llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp?rev=236916&r1=236915&r2=236916&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp (original)
> +++ llvm/trunk/lib/Target/ARM/ARMISelLowering.cpp Fri May 8 18:52:00 2015
> @@ -1847,8 +1847,10 @@ ARMTargetLowering::LowerCall(TargetLower
> Ops.push_back(InFlag);
>
> SDVTList NodeTys = DAG.getVTList(MVT::Other, MVT::Glue);
> - if (isTailCall)
> + if (isTailCall) {
> + MF.getFrameInfo()->setHasTailCall();
> return DAG.getNode(ARMISD::TC_RETURN, dl, NodeTys, Ops);
> + }
>
> // Returns a chain and a flag for retval copy to use.
> Chain = DAG.getNode(CallOpc, dl, NodeTys, Ops);
>
> Modified: llvm/trunk/lib/Target/Hexagon/HexagonISelLowering.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Hexagon/HexagonISelLowering.cpp?rev=236916&r1=236915&r2=236916&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/Hexagon/HexagonISelLowering.cpp (original)
> +++ llvm/trunk/lib/Target/Hexagon/HexagonISelLowering.cpp Fri May 8 18:52:00 2015
> @@ -637,8 +637,10 @@ HexagonTargetLowering::LowerCall(TargetL
> if (InFlag.getNode())
> Ops.push_back(InFlag);
>
> - if (isTailCall)
> + if (isTailCall) {
> + MF.getFrameInfo()->setHasTailCall();
> return DAG.getNode(HexagonISD::TC_RETURN, dl, NodeTys, Ops);
> + }
>
> int OpCode = doesNotReturn ? HexagonISD::CALLv3nr : HexagonISD::CALLv3;
> Chain = DAG.getNode(OpCode, dl, NodeTys, Ops);
>
> Modified: llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp?rev=236916&r1=236915&r2=236916&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp (original)
> +++ llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp Fri May 8 18:52:00 2015
> @@ -4215,6 +4215,7 @@ PPCTargetLowering::FinishCall(CallingCon
> isa<ConstantSDNode>(Callee)) &&
> "Expecting an global address, external symbol, absolute value or register");
>
> + MF.getFrameInfo()->setHasTailCall();
> return DAG.getNode(PPCISD::TC_RETURN, dl, MVT::Other, Ops);
> }
>
>
> Modified: llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/X86/X86ISelLowering.cpp?rev=236916&r1=236915&r2=236916&view=diff
> ==============================================================================
> --- llvm/trunk/lib/Target/X86/X86ISelLowering.cpp (original)
> +++ llvm/trunk/lib/Target/X86/X86ISelLowering.cpp Fri May 8 18:52:00 2015
> @@ -3135,6 +3135,7 @@ X86TargetLowering::LowerCall(TargetLower
> // This isn't right, although it's probably harmless on x86; liveouts
> // should be computed from returns not tail calls. Consider a void
> // function making a tail call to a function returning int.
> + MF.getFrameInfo()->setHasTailCall();
> return DAG.getNode(X86ISD::TC_RETURN, dl, NodeTys, Ops);
> }
>
>
> Added: llvm/trunk/test/CodeGen/AArch64/tailcall_misched_graph.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AArch64/tailcall_misched_graph.ll?rev=236916&view=auto
> ==============================================================================
> --- llvm/trunk/test/CodeGen/AArch64/tailcall_misched_graph.ll (added)
> +++ llvm/trunk/test/CodeGen/AArch64/tailcall_misched_graph.ll Fri May 8 18:52:00 2015
> @@ -0,0 +1,42 @@
> +; RUN: llc -mcpu=cyclone -debug-only=misched < %s 2>&1 | FileCheck %s
> +
> +; REQUIRE: asserts
> +
> +target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
> +target triple = "arm64-apple-ios7.0.0"
> +
> +define void @caller2(i8* %a0, i8* %a1, i8* %a2, i8* %a3, i8* %a4, i8* %a5, i8* %a6, i8* %a7, i8* %a8, i8* %a9) {
> +entry:
> + tail call void @callee2(i8* %a1, i8* %a2, i8* %a3, i8* %a4, i8* %a5, i8* %a6, i8* %a7, i8* %a8, i8* %a9, i8* %a0)
> + ret void
> +}
> +
> +declare void @callee2(i8*, i8*, i8*, i8*, i8*,
> + i8*, i8*, i8*, i8*, i8*)
> +
> +; Make sure there is a dependence between the load and store to the same stack
> +; location during a tail call. Tail calls clobber the incoming argument area and
> +; therefore it is not safe to assume argument locations are invariant.
> +; PR23459 has a test case that we where miscompiling because of this at the
> +; time.
> +
> +; CHECK: Frame Objects
> +; CHECK: fi#-4: {{.*}} fixed, at location [SP+8]
> +; CHECK: fi#-3: {{.*}} fixed, at location [SP]
> +; CHECK: fi#-2: {{.*}} fixed, at location [SP+8]
> +; CHECK: fi#-1: {{.*}} fixed, at location [SP]
> +
> +; CHECK: [[VRA:%vreg.*]]<def> = LDRXui <fi#-1>
> +; CHECK: [[VRB:%vreg.*]]<def> = LDRXui <fi#-2>
> +; CHECK: STRXui %vreg{{.*}}, <fi#-4>
> +; CHECK: STRXui [[VRB]], <fi#-3>
> +
> +; Make sure that there is an dependence edge between fi#-2 and fi#-4.
> +; Without this edge the scheduler would be free to move the store accross the load.
> +
> +; CHECK: SU({{.*}}): [[VRB]]<def> = LDRXui <fi#-2>
> +; CHECK-NOT: SU
> +; CHECK: Successors:
> +; CHECK: ch SU([[DEPSTORE:.*]]): Latency=0
> +
> +; CHECK: SU([[DEPSTORE]]): STRXui %vreg0, <fi#-4>
>
> Modified: llvm/trunk/test/CodeGen/ARM/debug-frame.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/debug-frame.ll?rev=236916&r1=236915&r2=236916&view=diff
> ==============================================================================
> --- llvm/trunk/test/CodeGen/ARM/debug-frame.ll (original)
> +++ llvm/trunk/test/CodeGen/ARM/debug-frame.ll Fri May 8 18:52:00 2015
> @@ -179,7 +179,7 @@ declare void @_ZSt9terminatev()
> ; CHECK-FP: .cfi_offset r4, -36
> ; CHECK-FP: add r11, sp, #28
> ; CHECK-FP: .cfi_def_cfa r11, 8
> -; CHECK-FP: sub sp, sp, #28
> +; CHECK-FP: sub sp, sp, #44
> ; CHECK-FP: .cfi_endproc
>
> ; CHECK-FP-ELIM-LABEL: _Z4testiiiiiddddd:
> @@ -195,8 +195,8 @@ declare void @_ZSt9terminatev()
> ; CHECK-FP-ELIM: .cfi_offset r6, -28
> ; CHECK-FP-ELIM: .cfi_offset r5, -32
> ; CHECK-FP-ELIM: .cfi_offset r4, -36
> -; CHECK-FP-ELIM: sub sp, sp, #28
> -; CHECK-FP-ELIM: .cfi_def_cfa_offset 64
> +; CHECK-FP-ELIM: sub sp, sp, #36
> +; CHECK-FP-ELIM: .cfi_def_cfa_offset 72
> ; CHECK-FP-ELIM: .cfi_endproc
>
> ; CHECK-V7-FP-LABEL: _Z4testiiiiiddddd:
>
> Modified: llvm/trunk/test/CodeGen/ARM/ehabi.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/ARM/ehabi.ll?rev=236916&r1=236915&r2=236916&view=diff
> ==============================================================================
> --- llvm/trunk/test/CodeGen/ARM/ehabi.ll (original)
> +++ llvm/trunk/test/CodeGen/ARM/ehabi.ll Fri May 8 18:52:00 2015
> @@ -146,8 +146,8 @@ declare void @_ZSt9terminatev()
> ; CHECK-FP: push {r4, r5, r6, r7, r8, r9, r10, r11, lr}
> ; CHECK-FP: .setfp r11, sp, #28
> ; CHECK-FP: add r11, sp, #28
> -; CHECK-FP: .pad #28
> -; CHECK-FP: sub sp, sp, #28
> +; CHECK-FP: .pad #44
> +; CHECK-FP: sub sp, sp, #44
> ; CHECK-FP: .personality __gxx_personality_v0
> ; CHECK-FP: .handlerdata
> ; CHECK-FP: .fnend
> @@ -156,8 +156,8 @@ declare void @_ZSt9terminatev()
> ; CHECK-FP-ELIM: .fnstart
> ; CHECK-FP-ELIM: .save {r4, r5, r6, r7, r8, r9, r10, r11, lr}
> ; CHECK-FP-ELIM: push {r4, r5, r6, r7, r8, r9, r10, r11, lr}
> -; CHECK-FP-ELIM: .pad #28
> -; CHECK-FP-ELIM: sub sp, sp, #28
> +; CHECK-FP-ELIM: .pad #36
> +; CHECK-FP-ELIM: sub sp, sp, #36
> ; CHECK-FP-ELIM: .personality __gxx_personality_v0
> ; CHECK-FP-ELIM: .handlerdata
> ; CHECK-FP-ELIM: .fnend
> @@ -205,7 +205,7 @@ declare void @_ZSt9terminatev()
> ; DWARF-FP: .cfi_offset r4, -36
> ; DWARF-FP: add r11, sp, #28
> ; DWARF-FP: .cfi_def_cfa r11, 8
> -; DWARF-FP: sub sp, sp, #28
> +; DWARF-FP: sub sp, sp, #44
> ; DWARF-FP: sub sp, r11, #28
> ; DWARF-FP: pop {r4, r5, r6, r7, r8, r9, r10, r11, lr}
> ; DWARF-FP: mov pc, lr
> @@ -226,9 +226,9 @@ declare void @_ZSt9terminatev()
> ; DWARF-FP-ELIM: .cfi_offset r6, -28
> ; DWARF-FP-ELIM: .cfi_offset r5, -32
> ; DWARF-FP-ELIM: .cfi_offset r4, -36
> -; DWARF-FP-ELIM: sub sp, sp, #28
> -; DWARF-FP-ELIM: .cfi_def_cfa_offset 64
> -; DWARF-FP-ELIM: add sp, sp, #28
> +; DWARF-FP-ELIM: sub sp, sp, #36
> +; DWARF-FP-ELIM: .cfi_def_cfa_offset 72
> +; DWARF-FP-ELIM: add sp, sp, #36
> ; DWARF-FP-ELIM: pop {r4, r5, r6, r7, r8, r9, r10, r11, lr}
> ; DWARF-FP-ELIM: mov pc, lr
> ; DWARF-FP-ELIM: .cfi_endproc
>
> Modified: llvm/trunk/test/CodeGen/X86/tailcallstack64.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/tailcallstack64.ll?rev=236916&r1=236915&r2=236916&view=diff
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/tailcallstack64.ll (original)
> +++ llvm/trunk/test/CodeGen/X86/tailcallstack64.ll Fri May 8 18:52:00 2015
> @@ -12,9 +12,9 @@
> ; Add %in1 %p1 to a different temporary register (%eax).
> ; CHECK: addl {{%edi|%ecx}}, [[R1]]
> ; Move param %in2 to stack.
> -; CHECK: movl [[R2]], [[A1]](%rsp)
> +; CHECK-DAG: movl [[R2]], [[A1]](%rsp)
> ; Move result of addition to stack.
> -; CHECK: movl [[R1]], [[A2]](%rsp)
> +; CHECK-DAG: movl [[R1]], [[A2]](%rsp)
> ; Eventually, do a TAILCALL
> ; CHECK: TAILCALL
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150508/02c0c8c7/attachment.html>
More information about the llvm-commits
mailing list