From: Sanjoy Das Date: Wed, 22 Jun 2016 22:16:51 +0000 (+0000) Subject: [ImplicitNullChecks] Hoist trivial depdendencies if possible X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=700dff73587cbeb92bcde5ea9564ffb43a6ca9dd;p=llvm [ImplicitNullChecks] Hoist trivial depdendencies if possible When trying to convert a loading instruction into a FAULTING_LOAD, we sometimes face code like this: if %R10 is not null: %R9 = MOV32ri Immediate %R9 = AND32rm %R9, 0x20(%R10) else: goto TRAP In these cases we would like to use the AND32rm instruction as the faulting operation by hoisting the "depedency" def-ing %R9 also above the control flow, transforming the program into: %R9 = MOV32ri Immediate %R9 = FAULTING_LOAD_OP(AND32rm %R9, 0x20(%R10), FailPath: TRAP) This change teaches ImplicitNullChecks to do the above, when safe. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@273501 91177308-0d34-0410-b5e6-96231b3b80d8 --- diff --git a/lib/CodeGen/ImplicitNullChecks.cpp b/lib/CodeGen/ImplicitNullChecks.cpp index ada66daac4c..11df2de216d 100644 --- a/lib/CodeGen/ImplicitNullChecks.cpp +++ b/lib/CodeGen/ImplicitNullChecks.cpp @@ -28,6 +28,7 @@ #include "llvm/ADT/DenseSet.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/Statistic.h" +#include "llvm/Analysis/AliasAnalysis.h" #include "llvm/CodeGen/Passes.h" #include "llvm/CodeGen/MachineFunction.h" #include "llvm/CodeGen/MachineMemOperand.h" @@ -75,14 +76,19 @@ class ImplicitNullChecks : public MachineFunctionPass { // The block branched to if the pointer is null. MachineBasicBlock *NullSucc; + // If this is non-null, then MemOperation has a dependency on on this + // instruction; and it needs to be hoisted to execute before MemOperation. + MachineInstr *OnlyDependency; + public: explicit NullCheck(MachineInstr *memOperation, MachineInstr *checkOperation, MachineBasicBlock *checkBlock, MachineBasicBlock *notNullSucc, - MachineBasicBlock *nullSucc) + MachineBasicBlock *nullSucc, + MachineInstr *onlyDependency) : MemOperation(memOperation), CheckOperation(checkOperation), - CheckBlock(checkBlock), NotNullSucc(notNullSucc), NullSucc(nullSucc) { - } + CheckBlock(checkBlock), NotNullSucc(notNullSucc), NullSucc(nullSucc), + OnlyDependency(onlyDependency) {} MachineInstr *getMemOperation() const { return MemOperation; } @@ -93,10 +99,13 @@ class ImplicitNullChecks : public MachineFunctionPass { MachineBasicBlock *getNotNullSucc() const { return NotNullSucc; } MachineBasicBlock *getNullSucc() const { return NullSucc; } + + MachineInstr *getOnlyDependency() const { return OnlyDependency; } }; const TargetInstrInfo *TII = nullptr; const TargetRegisterInfo *TRI = nullptr; + AliasAnalysis *AA = nullptr; MachineModuleInfo *MMI = nullptr; bool analyzeBlockForNullChecks(MachineBasicBlock &MBB, @@ -113,6 +122,10 @@ public: } bool runOnMachineFunction(MachineFunction &MF) override; + void getAnalysisUsage(AnalysisUsage &AU) const override { + AU.addRequired(); + MachineFunctionPass::getAnalysisUsage(AU); + } MachineFunctionProperties getRequiredProperties() const override { return MachineFunctionProperties().set( @@ -126,14 +139,22 @@ public: /// machine instruction can be re-ordered from after the machine instructions /// seen so far to before them. class HazardDetector { - DenseSet RegDefs; + static MachineInstr *getUnknownMI() { + return DenseMapInfo::getTombstoneKey(); + } + + // Maps physical registers to the instruction defining them. If there has + // been more than one def of an specific register, that register is mapped to + // getUnknownMI(). + DenseMap RegDefs; DenseSet RegUses; const TargetRegisterInfo &TRI; bool hasSeenClobber; + AliasAnalysis &AA; public: - explicit HazardDetector(const TargetRegisterInfo &TRI) : - TRI(TRI), hasSeenClobber(false) {} + explicit HazardDetector(const TargetRegisterInfo &TRI, AliasAnalysis &AA) + : TRI(TRI), hasSeenClobber(false), AA(AA) {} /// \brief Make a note of \p MI for later queries to isSafeToHoist. /// @@ -141,8 +162,10 @@ public: void rememberInstruction(MachineInstr *MI); /// \brief Return true if it is safe to hoist \p MI from after all the - /// instructions seen so far (via rememberInstruction) to before it. - bool isSafeToHoist(MachineInstr *MI); + /// instructions seen so far (via rememberInstruction) to before it. If \p MI + /// has one and only one transitive dependency, set \p Dependency to that + /// instruction. If there are more dependencies, return false. + bool isSafeToHoist(MachineInstr *MI, MachineInstr *&Dependency); /// \brief Return true if this instance of HazardDetector has been clobbered /// (i.e. has no more useful information). @@ -181,15 +204,23 @@ void HazardDetector::rememberInstruction(MachineInstr *MI) { if (!MO.isReg() || !MO.getReg()) continue; - if (MO.isDef()) - RegDefs.insert(MO.getReg()); - else + if (MO.isDef()) { + auto It = RegDefs.find(MO.getReg()); + if (It == RegDefs.end()) + RegDefs.insert({MO.getReg(), MI}); + else { + assert(It->second && "Found null MI?"); + It->second = getUnknownMI(); + } + } else RegUses.insert(MO.getReg()); } } -bool HazardDetector::isSafeToHoist(MachineInstr *MI) { +bool HazardDetector::isSafeToHoist(MachineInstr *MI, + MachineInstr *&Dependency) { assert(!isClobbered() && "isSafeToHoist cannot do anything useful!"); + Dependency = nullptr; // Right now we don't want to worry about LLVM's memory model. This can be // made more precise later. @@ -199,9 +230,54 @@ bool HazardDetector::isSafeToHoist(MachineInstr *MI) { for (auto &MO : MI->operands()) { if (MO.isReg() && MO.getReg()) { - for (unsigned Reg : RegDefs) - if (TRI.regsOverlap(Reg, MO.getReg())) - return false; // We found a write-after-write or read-after-write + for (auto &RegDef : RegDefs) { + unsigned Reg = RegDef.first; + MachineInstr *MI = RegDef.second; + if (!TRI.regsOverlap(Reg, MO.getReg())) + continue; + + // We found a write-after-write or read-after-write, see if the + // instruction causing this dependency can be hoisted too. + + if (MI == getUnknownMI()) + // We don't have precise dependency information. + return false; + + if (Dependency) { + if (Dependency == MI) + continue; + // We already have one dependency, and we can track only one. + return false; + } + + // Now check if MI is actually a dependency that can be hoisted. + + // We don't want to track transitive dependencies. We already know that + // MI is the only instruction that defines Reg, but we need to be sure + // that it does not use any registers that have been defined (trivially + // checked below by ensuring that there are no register uses), and that + // it is the only def for every register it defines (otherwise we could + // violate a write after write hazard). + auto IsMIOperandSafe = [&](MachineOperand &MO) { + if (!MO.isReg() || !MO.getReg()) + return true; + if (MO.isUse()) + return false; + assert((!MO.isDef() || RegDefs.count(MO.getReg())) && + "All defs must be tracked in RegDefs by now!"); + return !MO.isDef() || RegDefs.find(MO.getReg())->second == MI; + }; + + if (!all_of(MI->operands(), IsMIOperandSafe)) + return false; + + // Now check for speculation safety: + bool SawStore = true; + if (!MI->isSafeToMove(&AA, SawStore) || MI->mayLoad()) + return false; + + Dependency = MI; + } if (MO.isDef()) for (unsigned Reg : RegUses) @@ -217,6 +293,7 @@ bool ImplicitNullChecks::runOnMachineFunction(MachineFunction &MF) { TII = MF.getSubtarget().getInstrInfo(); TRI = MF.getRegInfo().getTargetRegisterInfo(); MMI = &MF.getMMI(); + AA = &getAnalysis().getAAResults(); SmallVector NullCheckList; @@ -229,6 +306,16 @@ bool ImplicitNullChecks::runOnMachineFunction(MachineFunction &MF) { return !NullCheckList.empty(); } +// Return true if any register aliasing \p Reg is live-in into \p MBB. +static bool AnyAliasLiveIn(const TargetRegisterInfo *TRI, + MachineBasicBlock *MBB, unsigned Reg) { + for (MCRegAliasIterator AR(Reg, TRI, /*IncludeSelf*/ true); AR.isValid(); + ++AR) + if (MBB->isLiveIn(*AR)) + return true; + return false; +} + /// Analyze MBB to check if its terminating branch can be turned into an /// implicit null check. If yes, append a description of the said null check to /// NullCheckList and return true, else return false. @@ -330,20 +417,56 @@ bool ImplicitNullChecks::analyzeBlockForNullChecks( unsigned PointerReg = MBP.LHS.getReg(); - HazardDetector HD(*TRI); + HazardDetector HD(*TRI, *AA); for (auto MII = NotNullSucc->begin(), MIE = NotNullSucc->end(); MII != MIE; ++MII) { MachineInstr *MI = &*MII; unsigned BaseReg; int64_t Offset; + MachineInstr *Dependency = nullptr; if (TII->getMemOpBaseRegImmOfs(MI, BaseReg, Offset, TRI)) if (MI->mayLoad() && !MI->isPredicable() && BaseReg == PointerReg && Offset < PageSize && MI->getDesc().getNumDefs() <= 1 && - HD.isSafeToHoist(MI)) { - NullCheckList.emplace_back(MI, MBP.ConditionDef, &MBB, NotNullSucc, - NullSucc); - return true; + HD.isSafeToHoist(MI, Dependency)) { + + auto DependencyOperandIsOk = [&](MachineOperand &MO) { + assert(!(MO.isReg() && MO.isUse()) && + "No transitive dependendencies please!"); + if (!MO.isReg() || !MO.getReg() || !MO.isDef()) + return true; + + // Make sure that we won't clobber any live ins to the sibling block + // by hoisting Dependency. For instance, we can't hoist INST to + // before the null check (even if it safe, and does not violate any + // dependencies in the non_null_block) if %rdx is live in to + // _null_block. + // + // test %rcx, %rcx + // je _null_block + // _non_null_block: + // %rdx = INST + // ... + if (AnyAliasLiveIn(TRI, NullSucc, MO.getReg())) + return false; + + // Make sure Dependency isn't re-defining the base register. Then we + // won't get the memory operation on the address we want. + if (TRI->regsOverlap(MO.getReg(), BaseReg)) + return false; + + return true; + }; + + bool DependencyOperandsAreOk = + !Dependency || + all_of(Dependency->operands(), DependencyOperandIsOk); + + if (DependencyOperandsAreOk) { + NullCheckList.emplace_back(MI, MBP.ConditionDef, &MBB, NotNullSucc, + NullSucc, Dependency); + return true; + } } HD.rememberInstruction(MI); @@ -399,6 +522,11 @@ void ImplicitNullChecks::rewriteNullChecks( (void)BranchesRemoved; assert(BranchesRemoved > 0 && "expected at least one branch!"); + if (auto *DepMI = NC.getOnlyDependency()) { + DepMI->removeFromParent(); + NC.getCheckBlock()->insert(NC.getCheckBlock()->end(), DepMI); + } + // Insert a faulting load where the conditional branch was originally. We // check earlier ensures that this bit of code motion is legal. We do not // touch the successors list for any basic block since we haven't changed @@ -418,6 +546,16 @@ void ImplicitNullChecks::rewriteNullChecks( continue; MBB->addLiveIn(Reg); } + + if (auto *DepMI = NC.getOnlyDependency()) { + for (auto &MO : DepMI->operands()) { + if (!MO.isReg() || !MO.getReg() || !MO.isDef()) + continue; + if (!NC.getNotNullSucc()->isLiveIn(MO.getReg())) + NC.getNotNullSucc()->addLiveIn(MO.getReg()); + } + } + NC.getMemOperation()->eraseFromParent(); NC.getCheckOperation()->eraseFromParent(); @@ -433,5 +571,6 @@ char ImplicitNullChecks::ID = 0; char &llvm::ImplicitNullChecksID = ImplicitNullChecks::ID; INITIALIZE_PASS_BEGIN(ImplicitNullChecks, "implicit-null-checks", "Implicit null checks", false, false) +INITIALIZE_PASS_DEPENDENCY(AAResultsWrapperPass) INITIALIZE_PASS_END(ImplicitNullChecks, "implicit-null-checks", "Implicit null checks", false, false) diff --git a/test/CodeGen/X86/implicit-null-checks.mir b/test/CodeGen/X86/implicit-null-checks.mir new file mode 100644 index 00000000000..ee8dc7d7629 --- /dev/null +++ b/test/CodeGen/X86/implicit-null-checks.mir @@ -0,0 +1,266 @@ +# RUN: llc -run-pass implicit-null-checks -mtriple=x86_64-apple-macosx -o /dev/null %s 2>&1 | FileCheck %s + +--- | + target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128" + target triple = "x86_64-apple-macosx" + + ;; Positive test + define i32 @imp_null_check_with_bitwise_op_0(i32* %x, i32 %val) { + entry: + br i1 undef, label %is_null, label %not_null, !make.implicit !0 + + is_null: + ret i32 42 + + not_null: + br i1 undef, label %ret_100, label %ret_200 + + ret_100: + ret i32 100 + + ret_200: + ret i32 200 + } + + ;; Negative test. The regalloc is such that we cannot hoist the + ;; instruction materializing 2200000 into %eax + define i32 @imp_null_check_with_bitwise_op_1(i32* %x, i32 %val, i32* %ptr) { + entry: + br i1 undef, label %is_null, label %not_null, !make.implicit !0 + + is_null: + ret i32 undef + + not_null: + br i1 undef, label %ret_100, label %ret_200 + + ret_100: + ret i32 100 + + ret_200: + ret i32 200 + } + + ;; Negative test: IR is identical to + ;; @imp_null_check_with_bitwise_op_0 but MIR differs. + define i32 @imp_null_check_with_bitwise_op_2(i32* %x, i32 %val) { + entry: + br i1 undef, label %is_null, label %not_null, !make.implicit !0 + + is_null: + ret i32 42 + + not_null: + br i1 undef, label %ret_100, label %ret_200 + + ret_100: + ret i32 100 + + ret_200: + ret i32 200 + } + + ;; Negative test: IR is identical to + ;; @imp_null_check_with_bitwise_op_0 but MIR differs. + define i32 @imp_null_check_with_bitwise_op_3(i32* %x, i32 %val) { + entry: + br i1 undef, label %is_null, label %not_null, !make.implicit !0 + + is_null: + ret i32 42 + + not_null: + br i1 undef, label %ret_100, label %ret_200 + + ret_100: + ret i32 100 + + ret_200: + ret i32 200 + } + + !0 = !{} +... +--- +name: imp_null_check_with_bitwise_op_0 +# CHECK-LABEL: name: imp_null_check_with_bitwise_op_0 +alignment: 4 +allVRegsAllocated: true +tracksRegLiveness: true +tracksSubRegLiveness: false +liveins: + - { reg: '%rdi' } + - { reg: '%esi' } +# CHECK: bb.0.entry: +# CHECK: %eax = MOV32ri 2200000 +# CHECK-NEXT: %eax = FAULTING_LOAD_OP %bb.3.is_null, 196, killed %eax, killed %rdi, 1, _, 0, _, implicit-def dead %eflags :: (load 4 from %ir.x) +# CHECK-NEXT: JMP_1 %bb.1.not_null + +body: | + bb.0.entry: + successors: %bb.3.is_null, %bb.1.not_null + liveins: %esi, %rdi + + TEST64rr %rdi, %rdi, implicit-def %eflags + JE_1 %bb.3.is_null, implicit %eflags + + bb.1.not_null: + successors: %bb.4.ret_100, %bb.2.ret_200 + liveins: %esi, %rdi + + %eax = MOV32ri 2200000 + %eax = AND32rm killed %eax, killed %rdi, 1, _, 0, _, implicit-def dead %eflags :: (load 4 from %ir.x) + CMP32rr killed %eax, killed %esi, implicit-def %eflags + JE_1 %bb.4.ret_100, implicit %eflags + + bb.2.ret_200: + %eax = MOV32ri 200 + RET 0, %eax + + bb.3.is_null: + %eax = MOV32ri 42 + RET 0, %eax + + bb.4.ret_100: + %eax = MOV32ri 100 + RET 0, %eax + +... +--- +name: imp_null_check_with_bitwise_op_1 +alignment: 4 +allVRegsAllocated: true +isSSA: false +tracksRegLiveness: true +tracksSubRegLiveness: false +liveins: + - { reg: '%rdi' } + - { reg: '%esi' } + - { reg: '%rdx' } +# CHECK: bb.0.entry: +# CHECK: %eax = MOV32rm killed %rdx, 1, _, 0, _ :: (volatile load 4 from %ir.ptr) +# CHECK-NEXT: TEST64rr %rdi, %rdi, implicit-def %eflags +# CHECK-NEXT: JE_1 %bb.3.is_null, implicit %eflags + +body: | + bb.0.entry: + successors: %bb.3.is_null, %bb.1.not_null + liveins: %esi, %rdi, %rdx + + %eax = MOV32rm killed %rdx, 1, _, 0, _ :: (volatile load 4 from %ir.ptr) + TEST64rr %rdi, %rdi, implicit-def %eflags + JE_1 %bb.3.is_null, implicit %eflags + + bb.1.not_null: + successors: %bb.4.ret_100, %bb.2.ret_200 + liveins: %esi, %rdi + + %eax = MOV32ri 2200000 + %eax = AND32rm killed %eax, killed %rdi, 1, _, 0, _, implicit-def dead %eflags :: (load 4 from %ir.x) + CMP32rr killed %eax, killed %esi, implicit-def %eflags + JE_1 %bb.4.ret_100, implicit %eflags + + bb.2.ret_200: + successors: %bb.3.is_null + + %eax = MOV32ri 200 + + bb.3.is_null: + liveins: %eax, %ah, %al, %ax, %bh, %bl, %bp, %bpl, %bx, %eax, %ebp, %ebx, %rax, %rbp, %rbx, %r12, %r13, %r14, %r15, %r12b, %r13b, %r14b, %r15b, %r12d, %r13d, %r14d, %r15d, %r12w, %r13w, %r14w, %r15w + + RET 0, %eax + + bb.4.ret_100: + %eax = MOV32ri 100 + RET 0, %eax + +... +--- +name: imp_null_check_with_bitwise_op_2 +# CHECK-LABEL: name: imp_null_check_with_bitwise_op_2 +alignment: 4 +allVRegsAllocated: true +tracksRegLiveness: true +tracksSubRegLiveness: false +liveins: + - { reg: '%rdi' } + - { reg: '%esi' } +# CHECK: bb.0.entry: +# CHECK: TEST64rr %rdi, %rdi, implicit-def %eflags +# CHECK-NEXT: JE_1 %bb.3.is_null, implicit %eflags + +body: | + bb.0.entry: + successors: %bb.3.is_null, %bb.1.not_null + liveins: %esi, %rdi + + TEST64rr %rdi, %rdi, implicit-def %eflags + JE_1 %bb.3.is_null, implicit %eflags + + bb.1.not_null: + successors: %bb.4.ret_100, %bb.2.ret_200 + liveins: %esi, %rdi + + %eax = MOV32ri 2200000 + %eax = ADD32ri killed %eax, 100, implicit-def dead %eflags + %eax = AND32rm killed %eax, killed %rdi, 1, _, 0, _, implicit-def dead %eflags :: (load 4 from %ir.x) + CMP32rr killed %eax, killed %esi, implicit-def %eflags + JE_1 %bb.4.ret_100, implicit %eflags + + bb.2.ret_200: + %eax = MOV32ri 200 + RET 0, %eax + + bb.3.is_null: + %eax = MOV32ri 42 + RET 0, %eax + + bb.4.ret_100: + %eax = MOV32ri 100 + RET 0, %eax + +... +--- +name: imp_null_check_with_bitwise_op_3 +# CHECK-LABEL: name: imp_null_check_with_bitwise_op_3 +alignment: 4 +allVRegsAllocated: true +tracksRegLiveness: true +tracksSubRegLiveness: false +liveins: + - { reg: '%rdi' } + - { reg: '%rsi' } +# CHECK: bb.0.entry: +# CHECK: TEST64rr %rdi, %rdi, implicit-def %eflags +# CHECK-NEXT: JE_1 %bb.3.is_null, implicit %eflags + +body: | + bb.0.entry: + successors: %bb.3.is_null, %bb.1.not_null + liveins: %rsi, %rdi + + TEST64rr %rdi, %rdi, implicit-def %eflags + JE_1 %bb.3.is_null, implicit %eflags + + bb.1.not_null: + successors: %bb.4.ret_100, %bb.2.ret_200 + liveins: %rsi, %rdi + + %rdi = MOV64ri 5000 + %rdi = AND64rm killed %rdi, killed %rdi, 1, _, 0, _, implicit-def dead %eflags :: (load 4 from %ir.x) + CMP64rr killed %rdi, killed %rsi, implicit-def %eflags + JE_1 %bb.4.ret_100, implicit %eflags + + bb.2.ret_200: + %eax = MOV32ri 200 + RET 0, %eax + + bb.3.is_null: + %eax = MOV32ri 42 + RET 0, %eax + + bb.4.ret_100: + %eax = MOV32ri 100 + RET 0, %eax + +...