From c61677f9d9b560c50a672aa1e758fbbf2077c44c Mon Sep 17 00:00:00 2001 From: Derek Schuff Date: Fri, 30 Sep 2016 18:02:54 +0000 Subject: [PATCH] [WebAssembly] Make register stackification more conservative Register stackification currently checks VNInfo for changes. Make that more accurate by testing each intervening instruction for any other defs to the same virtual register. Patch by Jacob Gravelle Differential Revision: https://reviews.llvm.org/D24942 git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@282886 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Target/WebAssembly/WebAssemblyRegStackify.cpp | 34 ++++++++++----------- test/CodeGen/WebAssembly/userstack.ll | 36 +++++++++++++++++++++++ 2 files changed, 51 insertions(+), 19 deletions(-) diff --git a/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp b/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp index 047ce4104f7..6dae653af59 100644 --- a/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp +++ b/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp @@ -274,11 +274,11 @@ static bool HasOneUse(unsigned Reg, MachineInstr *Def, // TODO: Compute memory dependencies in a way that uses AliasAnalysis to be // more precise. static bool IsSafeToMove(const MachineInstr *Def, const MachineInstr *Insert, - AliasAnalysis &AA, const LiveIntervals &LIS, - const MachineRegisterInfo &MRI) { + AliasAnalysis &AA, const MachineRegisterInfo &MRI) { assert(Def->getParent() == Insert->getParent()); // Check for register dependencies. + SmallVector MutableRegisters; for (const MachineOperand &MO : Def->operands()) { if (!MO.isReg() || MO.isUndef()) continue; @@ -301,21 +301,11 @@ static bool IsSafeToMove(const MachineInstr *Def, const MachineInstr *Insert, return false; } - // Ask LiveIntervals whether moving this virtual register use or def to - // Insert will change which value numbers are seen. - // - // If the operand is a use of a register that is also defined in the same - // instruction, test that the newly defined value reaches the insert point, - // since the operand will be moving along with the def. - const LiveInterval &LI = LIS.getInterval(Reg); - VNInfo *DefVNI = - (MO.isDef() || Def->definesRegister(Reg)) ? - LI.getVNInfoAt(LIS.getInstructionIndex(*Def).getRegSlot()) : - LI.getVNInfoBefore(LIS.getInstructionIndex(*Def)); - assert(DefVNI && "Instruction input missing value number"); - VNInfo *InsVNI = LI.getVNInfoBefore(LIS.getInstructionIndex(*Insert)); - if (InsVNI && DefVNI != InsVNI) - return false; + // If one of the operands isn't in SSA form, it has different values at + // different times, and we need to make sure we don't move our use across + // a different def. + if (!MO.isDef() && !MRI.hasOneDef(Reg)) + MutableRegisters.push_back(Reg); } bool Read = false, Write = false, Effects = false, StackPointer = false; @@ -323,7 +313,8 @@ static bool IsSafeToMove(const MachineInstr *Def, const MachineInstr *Insert, // If the instruction does not access memory and has no side effects, it has // no additional dependencies. - if (!Read && !Write && !Effects && !StackPointer) + bool HasMutableRegisters = !MutableRegisters.empty(); + if (!Read && !Write && !Effects && !StackPointer && !HasMutableRegisters) return true; // Scan through the intervening instructions between Def and Insert. @@ -343,6 +334,11 @@ static bool IsSafeToMove(const MachineInstr *Def, const MachineInstr *Insert, return false; if (StackPointer && InterveningStackPointer) return false; + + for (unsigned Reg : MutableRegisters) + for (const MachineOperand &MO : I->operands()) + if (MO.isReg() && MO.isDef() && MO.getReg() == Reg) + return false; } return true; @@ -781,7 +777,7 @@ bool WebAssemblyRegStackify::runOnMachineFunction(MachineFunction &MF) { // supports intra-block moves) and it's MachineSink's job to catch all // the sinking opportunities anyway. bool SameBlock = Def->getParent() == &MBB; - bool CanMove = SameBlock && IsSafeToMove(Def, Insert, AA, LIS, MRI) && + bool CanMove = SameBlock && IsSafeToMove(Def, Insert, AA, MRI) && !TreeWalker.IsOnStack(Reg); if (CanMove && HasOneUse(Reg, Def, MRI, MDT, LIS)) { Insert = MoveForSingleUse(Reg, Op, Def, MBB, Insert, LIS, MFI, MRI); diff --git a/test/CodeGen/WebAssembly/userstack.ll b/test/CodeGen/WebAssembly/userstack.ll index 234e647f196..44c5da53716 100644 --- a/test/CodeGen/WebAssembly/userstack.ll +++ b/test/CodeGen/WebAssembly/userstack.ll @@ -232,6 +232,42 @@ define void @dynamic_static_alloca(i32 %alloc) noredzone { ret void } +declare i8* @llvm.stacksave() +declare void @llvm.stackrestore(i8*) + +; CHECK-LABEL: llvm_stack_builtins: +define void @llvm_stack_builtins(i32 %alloc) noredzone { + ; CHECK: i32.load $push[[L11:.+]]=, __stack_pointer($pop{{.+}}) + ; CHECK-NEXT: tee_local $push[[L10:.+]]=, ${{.+}}=, $pop[[L11]] + ; CHECK-NEXT: copy_local $[[STACK:.+]]=, $pop[[L10]] + %stack = call i8* @llvm.stacksave() + + ; Ensure we don't reassign the stacksave local + ; CHECK-NOT: $[[STACK]]= + %dynamic = alloca i32, i32 %alloc + + ; CHECK: i32.store $drop=, __stack_pointer($pop{{.+}}), $[[STACK]] + call void @llvm.stackrestore(i8* %stack) + + ret void +} + +; Not actually using the alloca'd variables exposed an issue with register +; stackification, where copying the stack pointer into the frame pointer was +; moved after the stack pointer was updated for the dynamic alloca. +; CHECK-LABEL: dynamic_alloca_nouse: +define void @dynamic_alloca_nouse(i32 %alloc) noredzone { + ; CHECK: i32.load $push[[L11:.+]]=, __stack_pointer($pop{{.+}}) + ; CHECK-NEXT: tee_local $push[[L10:.+]]=, ${{.+}}=, $pop[[L11]] + ; CHECK-NEXT: copy_local $[[FP:.+]]=, $pop[[L10]] + %dynamic = alloca i32, i32 %alloc + + ; CHECK-NOT: $[[FP]]=, + + ; CHECK: i32.store $drop=, __stack_pointer($pop{{.+}}), $[[FP]] + ret void +} + ; The use of the alloca in a phi causes a CopyToReg DAG node to be generated, ; which has to have special handling because CopyToReg can't have a FI operand ; CHECK-LABEL: copytoreg_fi: -- 2.11.0