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); @@ -162,16 +163,20 @@ enum SuitabilityResult { SR_Suitable, SR_Unsuitable, SR_Impossible }; + /// Returns SR_Suitable if \p MI memory operation does not alias with + /// \p PrevMI, SR_Unsuitable if they may alias and SR_Impossible if they may + /// alias and any further memory operation may alias with \p PrevMI. + ImplicitNullChecks::SuitabilityResult + AliasMemoryOp(MachineInstr &MI, MachineInstr *PrevMI); + /// 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 +274,7 @@ TII = MF.getSubtarget().getInstrInfo(); TRI = MF.getRegInfo().getTargetRegisterInfo(); MMI = &MF.getMMI(); + MFI = &MF.getFrameInfo(); AA = &getAnalysis().getAAResults(); SmallVector NullCheckList; @@ -293,46 +299,76 @@ } ImplicitNullChecks::SuitabilityResult +ImplicitNullChecks::AliasMemoryOp(MachineInstr &MI, MachineInstr *PrevMI) { + // If it is not memory access, skip the check. + if (!(PrevMI->mayStore() || PrevMI->mayLoad())) + return SR_Suitable; + // Load-Load may alias + if (!(MI.mayStore() || PrevMI->mayStore())) + return SR_Suitable; + // 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() ? SR_Impossible : SR_Unsuitable; + if (PrevMI->memoperands_empty()) + return PrevMI->mayStore() ? SR_Impossible : SR_Unsuitable; + + for (MachineMemOperand *MMO1 : MI.memoperands()) + for (MachineMemOperand *MMO2 : PrevMI->memoperands()) { + if (const PseudoSourceValue *PSV = MMO2->getPseudoValue()) + return PSV->mayAlias(MFI) ? SR_Unsuitable : SR_Suitable; + // Not a Value, conservatively alias. + if (!MMO1->getValue()) + return SR_Unsuitable; + AliasResult AAResult = AA->alias( + MemoryLocation(MMO1->getValue(), MemoryLocation::UnknownSize, + MMO1->getAAInfo()), + MemoryLocation(MMO2->getValue(), MemoryLocation::UnknownSize, + MMO2->getAAInfo())); + if(AAResult != NoAlias) + return SR_Unsuitable; + } + return SR_Suitable; +} + +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) { + Suitable = AliasMemoryOp(MI, PrevMI); + if (Suitable == SR_Impossible) + return SR_Impossible; + } + } + return Suitable; } bool ImplicitNullChecks::canHoistInst(MachineInstr *FaultingMI, @@ -503,7 +539,6 @@ const unsigned PointerReg = MBP.LHS.getReg(); SmallVector InstsSeenSoFar; - bool SeenLoad = false; for (auto &MI : *NotNullSucc) { if (!canHandle(&MI) || InstsSeenSoFar.size() >= MaxInstsToConsider) @@ -511,7 +546,7 @@ MachineInstr *Dependence; SuitabilityResult SR = - isSuitableMemoryOp(MI, PointerReg, InstsSeenSoFar, SeenLoad); + 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 + +...