Index: lib/CodeGen/ImplicitNullChecks.cpp =================================================================== --- lib/CodeGen/ImplicitNullChecks.cpp +++ lib/CodeGen/ImplicitNullChecks.cpp @@ -153,6 +153,7 @@ const TargetRegisterInfo *TRI = nullptr; AliasAnalysis *AA = nullptr; MachineModuleInfo *MMI = nullptr; + MachineFrameInfo *MFI = nullptr; bool analyzeBlockForNullChecks(MachineBasicBlock &MBB, SmallVectorImpl &NullCheckList); @@ -160,18 +161,29 @@ MachineBasicBlock *HandlerMBB); void rewriteNullChecks(ArrayRef NullCheckList); - enum SuitabilityResult { SR_Suitable, SR_Unsuitable, SR_Impossible }; - + enum AliasResult { + AR_NoAlias, + AR_MayAlias, + AR_WillAliasEverything + }; + /// Returns AR_NoAlias if \p MI memory operation does not alias with + /// \p PrevMI, AR_MayAlias if they may alias and AR_WillAliasEverything if + /// they may alias and any further memory operation may alias with \p PrevMI. + AliasResult areMemoryOpsAliased(MachineInstr &MI, MachineInstr *PrevMI); + + enum SuitabilityResult { + SR_Suitable, + SR_Unsuitable, + SR_Impossible + }; /// Return SR_Suitable if \p MI a memory operation that can be used to /// implicitly null check the value in \p PointerReg, SR_Unsuitable if /// \p MI cannot be used to null check and SR_Impossible if there is /// no sense to continue lookup due to any other instruction will not be able /// to be used. \p PrevInsts is the set of instruction seen since - /// the explicit null check on \p PointerReg. \p SeenLoad means that load - /// instruction has been observed in \PrevInsts set. + /// the explicit null check on \p PointerReg. SuitabilityResult isSuitableMemoryOp(MachineInstr &MI, unsigned PointerReg, - ArrayRef PrevInsts, - bool &SeenLoad); + ArrayRef PrevInsts); /// Return true if \p FaultingMI can be hoisted from after the the /// instructions in \p InstsSeenSoFar to before them. Set \p Dependence to a @@ -269,6 +281,7 @@ TII = MF.getSubtarget().getInstrInfo(); TRI = MF.getRegInfo().getTargetRegisterInfo(); MMI = &MF.getMMI(); + MFI = &MF.getFrameInfo(); AA = &getAnalysis().getAAResults(); SmallVector NullCheckList; @@ -292,47 +305,84 @@ return false; } +ImplicitNullChecks::AliasResult +ImplicitNullChecks::areMemoryOpsAliased(MachineInstr &MI, + MachineInstr *PrevMI) { + // If it is not memory access, skip the check. + if (!(PrevMI->mayStore() || PrevMI->mayLoad())) + return AR_NoAlias; + // Load-Load may alias + if (!(MI.mayStore() || PrevMI->mayStore())) + return AR_NoAlias; + // We lost info, conservatively alias. If it was store then no sense to + // continue because we won't be able to check against it further. + if (MI.memoperands_empty()) + return MI.mayStore() ? AR_WillAliasEverything : AR_MayAlias; + if (PrevMI->memoperands_empty()) + return PrevMI->mayStore() ? AR_WillAliasEverything : AR_MayAlias; + + for (MachineMemOperand *MMO1 : MI.memoperands()) { + // MMO1 should have a value due it comes from operation we'd like to use + // as implicit null check. + assert(MMO1->getValue() && "MMO1 should have a Value!"); + for (MachineMemOperand *MMO2 : PrevMI->memoperands()) { + if (const PseudoSourceValue *PSV = MMO2->getPseudoValue()) { + if (PSV->mayAlias(MFI)) + return AR_MayAlias; + continue; + } + llvm::AliasResult AAResult = AA->alias( + MemoryLocation(MMO1->getValue(), MemoryLocation::UnknownSize, + MMO1->getAAInfo()), + MemoryLocation(MMO2->getValue(), MemoryLocation::UnknownSize, + MMO2->getAAInfo())); + if (AAResult != NoAlias) + return AR_MayAlias; + } + } + return AR_NoAlias; +} + ImplicitNullChecks::SuitabilityResult ImplicitNullChecks::isSuitableMemoryOp(MachineInstr &MI, unsigned PointerReg, - ArrayRef PrevInsts, - bool &SeenLoad) { + ArrayRef PrevInsts) { int64_t Offset; unsigned BaseReg; - // First, if it is a store and we saw load before we bail out - // because we will not be able to re-order load-store without - // using alias analysis. - if (SeenLoad && MI.mayStore()) - return SR_Impossible; - - SeenLoad = SeenLoad || MI.mayLoad(); - - // Without alias analysis we cannot re-order store with anything. - // so if this instruction is not a candidate we should stop. - SuitabilityResult Unsuitable = MI.mayStore() ? SR_Impossible : SR_Unsuitable; - if (!TII->getMemOpBaseRegImmOfs(MI, BaseReg, Offset, TRI) || BaseReg != PointerReg) - return Unsuitable; + return SR_Unsuitable; // We want the mem access to be issued at a sane offset from PointerReg, // so that if PointerReg is null then the access reliably page faults. if (!((MI.mayLoad() || MI.mayStore()) && !MI.isPredicable() && Offset < PageSize)) - return Unsuitable; + return SR_Unsuitable; // Finally, we need to make sure that the access instruction actually is // accessing from PointerReg, and there isn't some re-definition of PointerReg // between the compare and the memory access. // If PointerReg has been redefined before then there is no sense to continue // lookup due to this condition will fail for any further instruction. + SuitabilityResult Suitable = SR_Suitable; for (auto *PrevMI : PrevInsts) - for (auto &PrevMO : PrevMI->operands()) + for (auto &PrevMO : PrevMI->operands()) { if (PrevMO.isReg() && PrevMO.getReg() && PrevMO.isDef() && TRI->regsOverlap(PrevMO.getReg(), PointerReg)) return SR_Impossible; - return SR_Suitable; + // Check whether the current memory access aliases with previous one. + // If we already found that it aliases then no need to continue. + // But we continue base pointer check as it can result in SR_Impossible. + if (Suitable == SR_Suitable) { + AliasResult AR = areMemoryOpsAliased(MI, PrevMI); + if (AR == AR_WillAliasEverything) + return SR_Impossible; + if (AR == AR_MayAlias) + Suitable = SR_Unsuitable; + } + } + return Suitable; } bool ImplicitNullChecks::canHoistInst(MachineInstr *FaultingMI, @@ -503,15 +553,13 @@ const unsigned PointerReg = MBP.LHS.getReg(); SmallVector InstsSeenSoFar; - bool SeenLoad = false; for (auto &MI : *NotNullSucc) { if (!canHandle(&MI) || InstsSeenSoFar.size() >= MaxInstsToConsider) return false; MachineInstr *Dependence; - SuitabilityResult SR = - isSuitableMemoryOp(MI, PointerReg, InstsSeenSoFar, SeenLoad); + SuitabilityResult SR = isSuitableMemoryOp(MI, PointerReg, InstsSeenSoFar); if (SR == SR_Impossible) return false; if (SR == SR_Suitable && Index: test/CodeGen/X86/implicit-null-checks.mir =================================================================== --- test/CodeGen/X86/implicit-null-checks.mir +++ test/CodeGen/X86/implicit-null-checks.mir @@ -341,6 +341,30 @@ ret void } + define i32 @inc_store_and_load_no_alias(i32* noalias %ptr, i32* noalias %ptr2) { + entry: + %ptr_is_null = icmp eq i32* %ptr, null + br i1 %ptr_is_null, label %is_null, label %not_null, !make.implicit !0 + + not_null: + ret i32 undef + + is_null: + ret i32 undef + } + + define i32 @inc_store_and_load_alias(i32* %ptr, i32* %ptr2) { + entry: + %ptr_is_null = icmp eq i32* %ptr, null + br i1 %ptr_is_null, label %is_null, label %not_null, !make.implicit !0 + + not_null: + ret i32 undef + + is_null: + ret i32 undef + } + attributes #0 = { "target-features"="+bmi,+bmi2" } !0 = !{} @@ -645,7 +669,7 @@ name: use_alternate_load_op # CHECK-LABEL: name: use_alternate_load_op # CHECK: bb.0.entry: -# CHECK: %r10 = FAULTING_OP 1, %bb.2.is_null, {{[0-9]+}}, killed %rdi, 1, _, 0, _ +# CHECK: %rax = FAULTING_OP 1, %bb.2.is_null, {{[0-9]+}}, killed %rdi, 1, _, 0, _ # CHECK-NEXT: JMP_1 %bb.1.not_null # CHECK: bb.1.not_null @@ -666,9 +690,9 @@ liveins: %rdi, %rsi %rcx = MOV64rm killed %rsi, 1, _, 0, _ - %rdx = AND64rm killed %rcx, %rdi, 1, _, 0, _, implicit-def dead %eflags - %r10 = MOV64rm killed %rdi, 1, _, 0, _ - RETQ %r10d + %rcx = AND64rm killed %rcx, %rdi, 1, _, 0, _, implicit-def dead %eflags + %rax = MOV64rm killed %rdi, 1, _, 0, _ + RETQ %eax bb.2.is_null: %eax = XOR32rr undef %eax, undef %eax, implicit-def dead %eflags @@ -1207,3 +1231,69 @@ RETQ ... +--- +name: inc_store_and_load_no_alias +# CHECK-LABEL: inc_store_and_load_no_alias +# CHECK: bb.0.entry: +# CHECK: %eax = FAULTING_OP 1, %bb.2.is_null, {{[0-9]+}}, killed %rdi, 1, _, 0, _ :: (load 4 from %ir.ptr) +# CHECK-NEXT: JMP_1 %bb.1.not_null +# CHECK: bb.1.not_null + +alignment: 4 +tracksRegLiveness: true +liveins: + - { reg: '%rdi' } + - { reg: '%rsi' } +body: | + bb.0.entry: + successors: %bb.2.is_null, %bb.1.not_null + liveins: %rdi, %rsi + + TEST64rr %rdi, %rdi, implicit-def %eflags + JE_1 %bb.2.is_null, implicit killed %eflags + + bb.1.not_null: + liveins: %rdi, %rsi + + MOV32mi killed %rsi, 1, _, 0, _, 3 :: (store 4 into %ir.ptr2) + %eax = MOV32rm killed %rdi, 1, _, 0, _ :: (load 4 from %ir.ptr) + RETQ %eax + + bb.2.is_null: + %eax = XOR32rr undef %eax, undef %eax, implicit-def dead %eflags + RETQ %eax + +... +--- +name: inc_store_and_load_alias +# CHECK-LABEL: inc_store_and_load_alias +# CHECK: bb.0.entry: +# CHECK: TEST64rr %rdi, %rdi, implicit-def %eflags +# CHECK-NEXT: JE_1 %bb.2.is_null, implicit killed %eflags +# CHECK: bb.1.not_null + +alignment: 4 +tracksRegLiveness: true +liveins: + - { reg: '%rdi' } + - { reg: '%rsi' } +body: | + bb.0.entry: + successors: %bb.2.is_null, %bb.1.not_null + liveins: %rdi, %rsi + + TEST64rr %rdi, %rdi, implicit-def %eflags + JE_1 %bb.2.is_null, implicit killed %eflags + + bb.1.not_null: + liveins: %rdi, %rsi + + MOV32mi killed %rsi, 1, _, 0, _, 3 :: (store 4 into %ir.ptr2) + %eax = MOV32rm killed %rdi, 1, _, 0, _ :: (load 4 from %ir.ptr) + RETQ %eax + + bb.2.is_null: + %eax = XOR32rr undef %eax, undef %eax, implicit-def dead %eflags + RETQ %eax + +...