[ImplicitNullCheck] Handle Nonzero faulting pages and complex addressing

[anna]

Summary

This patch:

1. introduces a new TII API for calculating the address of a memory operation in complex addressing mode. This is a general function, which checks for both bas and index registers. This is used in isSuitableMemoryOp in ImplicitNullChecks and all the validity checks are done in this caller function. Earlier, we used to bail out if the memory operation had an index register or a scale != 1 (i.e. we supported offsets with simple addressing mode).
2. handles memory accesses (and makes the null checks implicit) where the faulting page is a non-zero one as

specified through a named module level metadata startaddress_faulting_pages introduced in this patch. The optional metadata is added by front-ends.

See #2 in action with added MIR tests for positive and negative case. #1
allows us to handle a case which was not handled earlier (see
implicit-null-checks.ll).

[anna]

While working on a follow-up change, I realized we don't need to separate out whether we're handling simple or complex addressing mode based on whether we fault in zero page or not. Both are orthogonal issues. The reason I coupled them is because we still need a way to get the "total constant address" *if it exists* and make a distinction between when we succeeded to get one versus when there was no such constant address apart from the offset.

[anna]

handles complex addressing mode using base and index operands. All test cases pass.
I plan to add more counter examples once the basic idea is agreed upon.

[anna]

llvm/test/CodeGen/X86/implicit-null-check-negative.ll

132

I added these to show that none of the negative test cases start failing once we add the metadata with "other faulting pages".

[dantrushin]

llvm/include/llvm/CodeGen/TargetInstrInfo.h

1305

These names are confusing. I've been scrolling back and forth for several minutes :)
Why not to pass and name them separately?

llvm/lib/CodeGen/ImplicitNullChecks.cpp

390

Do you really need to check that BaseRegUsedInConstAddr.first is not zero?
Would simple BaseRegUsedInConstAddr.first != PointerReg work here?

I also found positive names more readable, e.g. bool BaseRegIsNullChecked = BaseRegUsedInConstAddr.first == PointerReg

I think it is reasonable to assert Scale !=0 || IndexRegUsedInConstAddr.first == 0 and remove Scale check.

llvm/lib/Target/X86/X86InstrInfo.cpp

3680

Any reason to use auto * instead of auto & ?
If I read it correctly, BaseOp is used to get base register only. Using reference you'd have less characters to type :)

3693

ditto. auto &IndexOp = MI.getOperand() ?

[anna]

addressed review comments. Added more testcases (positive and countercases).

[anna]

llvm/lib/CodeGen/ImplicitNullChecks.cpp

390

Changed the names and hopefully more readable.
We need to check that the register exists before checking if it is equal to the null checked register because we don't want to fail when we don't have the register (in that case, we wont be using it in the addres calculation). Here we will state that the memory op is unsuitable and then fail. I realized this through some failing test case imp_null_check_load in implicit-null-checks.ll. There, we do not use the index register, so it is zero.

[dantrushin]

Thanks, it reads much better for me now.
Looks good.

[reames]

Must fix:

• Your change does not include a LangRef update for the new metadata.
• The name chosen for the metadata is ambiguous. I'd also suggest phrasing it in terms of ranges, not page starts. Maybe: guaranteed_faulting_ranges, or implicit_check_faulting_ranges?
• As you note in the review, this is mixing two sets of changes in a way which makes it hard to reason about either in isolation. Please split. Either order is fine as you should be able to test either piece in isolation without the other.

llvm/include/llvm/CodeGen/TargetInstrInfo.h

1301

I find the terminology used here to be very confusing. A constant address to me is either a literal constant or a constant expression. You appear to be referring to complex addressing instead. I believe that your ConstantAddr is the Displacement field in a x86 complex addressing, is that true?

If so, rename required. If not, please explain.

llvm/lib/CodeGen/ImplicitNullChecks.cpp

476

From your comment, it sounds like the existing code handles two built in ranges, not one.
0, PageSize
0x100..00, 0x111..11

(i.e. the zeroth page and kernel address range for Linux)

I'd suggest adjusting comments and code structure to reflect same, and to make range handling (not page starts) explicit.

llvm/lib/Target/X86/X86InstrInfo.cpp

3701

This really doesn't seem like it belongs here. I think this could be cleanly moved into the caller, simplifying the behaviour of this routine greatly.

[anna]

addressed review comments about splitting out patches. This patch handles complex addressing mode.

[reames]

Looks close to ready. Please address review comments, and we'll probably be ready for an LGTM.

llvm/include/llvm/CodeGen/TargetInstrInfo.h

980

"is an instruction"

llvm/lib/CodeGen/ImplicitNullChecks.cpp

406

These should be impossible, use asserts.

424

There's a potential overflow case here which needs consideration. Consider 0xF...F as constant and scale as 2.

You probably need to return some information about the register width/wrapping of the addressing mode to do this in a target independent way.

428

Please return true or false, then set the per register flag. This is too much coupling.

[anna]

llvm/lib/CodeGen/ImplicitNullChecks.cpp

406

Will use assert for the multiplier and add a comment why RegUsedInAddr can be zero reg, i.e. something like: movq 8(,%rdi,8), %rax here the BaseReg is X86::NoRegister and ScaleReg is rdi.

424

good catch! will take a look.

428

yes, agreed.

[anna]

addressed review comments. Bails out in case of overflow (added testcase for multiply overflow).

[reames]

LGTM, but with required changes.

llvm/lib/CodeGen/ImplicitNullChecks.cpp

393

Please add a check here that the size of BaseReg, ScaledReg, and PointerReg are all equal.

I'm concerned about implicit sign extension of registers, and the interaction with the displacement code below. The code you have may be correct, but I'd prefer a explicit bail out just to make it easier to tell.

405

You could if you wished do Displacement mod 2^64, but I don't see any point in that.

411

Add comment here: "If a register used in the address is constant, fold it's effect into the displacement for ease of analysis."

432

Replace IsSignedImmVal with constant true.

This isn't "is negative", this is "interpret this as a signed number instead of unsigned".

434

You should be able to just define the APInt in the definition below. The bitwidth will come from the RHS.

441

e,g, APInt Product =

Also, remember to constant fold isSignedImmVal

llvm/test/CodeGen/X86/implicit-null-check.ll

625

You don't appear to actually have a test here which uses a displacement large enough to need a base register, please add one.

(i.e. modify this test to add not 3526, but 0xFFFFF0000 (or something similarly large)

[anna]

llvm/test/CodeGen/X86/implicit-null-check.ll

625

I have a test in the negative test cases file above where the displacement is large and outside the faulting page. I'll modify it to a very large value so that it is in a register and not an immediate.

[anna]

addressed review comments, rebased and updated for AArch64 (this patch is NFC in behaviour for AArch64).

[GMNGeoffrey]

Hey JFYI this created some unused variables. I fixed with https://reviews.llvm.org/D89022

llvm/include/llvm/CodeGen/TargetInstrInfo.h

 /// Used to describe a register and immediate addition.
 struct RegImmPair {
   Register Reg;
   int64_t Imm;
 
   RegImmPair(Register Reg, int64_t Imm) : Reg(Reg), Imm(Imm) {}
 };
 
+/// Used to describe addressing mode similar to ExtAddrMode in CodeGenPrepare.
+/// It holds the register values, the scale value and the displacement.
+struct ExtAddrMode {
+  Register BaseReg;
+  Register ScaledReg;
+  int64_t Scale;
+  int64_t Displacement;
+};
+
 //---------------------------------------------------------------------------
 ///
 /// TargetInstrInfo - Interface to description of machine instruction set
 ///
 class TargetInstrInfo : public MCInstrInfo {
 public:
   TargetInstrInfo(unsigned CFSetupOpcode = ~0u, unsigned CFDestroyOpcode = ~0u,
                   unsigned CatchRetOpcode = ~0u, unsigned ReturnOpcode = ~0u)
public:
   /// immediate value and a physical register, and stores the result in
   /// the given physical register \c Reg, return a pair of the source
   /// register and the offset which has been added.
   virtual Optional<RegImmPair> isAddImmediate(const MachineInstr &MI,
                                               Register Reg) const {
     return None;
   }
 
+  /// Returns true if MI is an instruction that defines Reg to have a constant
+  /// value and the value is recorded in ImmVal. The ImmVal is a result that
+  /// should be interpreted as modulo size of Reg.
+  virtual bool getConstValDefinedInReg(const MachineInstr &MI,
+                                       const Register Reg,
+                                       int64_t &ImmVal) const {
+    return false;
+  }
+
   /// Store the specified register of the given register class to the specified
   /// stack frame index. The store instruction is to be added to the given
   /// machine basic block before the specified machine instruction. If isKill
   /// is true, the register operand is the last use and must be marked kill.
   virtual void storeRegToStackSlot(MachineBasicBlock &MBB,
                                    MachineBasicBlock::iterator MI,
                                    Register SrcReg, bool isKill, int FrameIndex,
                                    const TargetRegisterClass *RC,
public:
   /// true, the function also sets the operand position in the instruction
   /// for the base register and offset.
   virtual bool getBaseAndOffsetPosition(const MachineInstr &MI,
                                         unsigned &BasePos,
                                         unsigned &OffsetPos) const {
     return false;
   }
 
+  /// Target dependent implementation to get the values constituting the address
+  /// MachineInstr that is accessing memory. These values are returned as a
+  /// struct ExtAddrMode which contains all relevant information to make up the
+  /// address.
+  virtual Optional<ExtAddrMode>
+  getAddrModeFromMemoryOp(const MachineInstr &MemI,
+                          const TargetRegisterInfo *TRI) const {
+    return None;
+  }
+
   /// Returns true if MI's Def is NullValueReg, and the MI
   /// does not change the Zero value. i.e. cases such as rax = shr rax, X where
   /// NullValueReg = rax. Note that if the NullValueReg is non-zero, this
   /// function can return true even if becomes zero. Specifically cases such as
   /// NullValueReg = shl NullValueReg, 63.
   virtual bool preservesZeroValueInReg(const MachineInstr *MI,
                                        const Register NullValueReg,
                                        const TargetRegisterInfo *TRI) const {
     return false;
   }
 
   /// If the instruction is an increment of a constant value, return the amount.
   virtual bool getIncrementValue(const MachineInstr &MI, int &Value) const {

llvm/lib/CodeGen/ImplicitNullChecks.cpp

ImplicitNullChecks::isSuitableMemoryOp(const MachineInstr &MI,
   bool OffsetIsScalable;
   const MachineOperand *BaseOp;
 
   // Implementation restriction for faulting_op insertion
   // TODO: This could be relaxed if we find a test case which warrants it.
   if (MI.getDesc().getNumDefs() > 1)
    return SR_Unsuitable;
 
-  // FIXME: This handles only simple addressing mode.
-  if (!TII->getMemOperandWithOffset(MI, BaseOp, Offset, OffsetIsScalable, TRI))
+  if (!MI.mayLoadOrStore() || MI.isPredicable())
+    return SR_Unsuitable;
+  auto AM = TII->getAddrModeFromMemoryOp(MI, TRI);
+  if (!AM)
     return SR_Unsuitable;
+  auto AddrMode = *AM;
+  const Register BaseReg = AddrMode.BaseReg, ScaledReg = AddrMode.ScaledReg;
+  int64_t Displacement = AddrMode.Displacement;
 
   // We need the base of the memory instruction to be same as the register
   // where the null check is performed (i.e. PointerReg).
-  if (!BaseOp->isReg() || BaseOp->getReg() != PointerReg)
+  if (BaseReg != PointerReg && ScaledReg != PointerReg)
     return SR_Unsuitable;
-
-  // Scalable offsets are a part of scalable vectors (SVE for AArch64). That
-  // target is in-practice unsupported for ImplicitNullChecks.
-  if (OffsetIsScalable)
+  const MachineRegisterInfo &MRI = MI.getMF()->getRegInfo();
+  unsigned PointerRegSizeInBits = TRI->getRegSizeInBits(PointerReg, MRI);
+  // Bail out of the sizes of BaseReg, ScaledReg and PointerReg are not the
+  // same.
+  if ((BaseReg &&
+       TRI->getRegSizeInBits(BaseReg, MRI) != PointerRegSizeInBits) ||
+      (ScaledReg &&
+       TRI->getRegSizeInBits(ScaledReg, MRI) != PointerRegSizeInBits))
     return SR_Unsuitable;
 
-  if (!MI.mayLoadOrStore() || MI.isPredicable())
+  // Returns true if RegUsedInAddr is used for calculating the displacement
+  // depending on addressing mode. Also calculates the Displacement.
+  auto CalculateDisplacementFromAddrMode = [&](Register RegUsedInAddr,
+                                               int64_t Multiplier) {
+    // The register can be NoRegister, which is defined as zero for all targets.
+    // Consider instruction of interest as `movq 8(,%rdi,8), %rax`. Here the
+    // ScaledReg is %rdi, while there is no BaseReg.
+    if (!RegUsedInAddr)
+      return false;
+    assert(Multiplier && "expected to be non-zero!");
+    MachineInstr *ModifyingMI = nullptr;
+    for (auto It = std::next(MachineBasicBlock::const_reverse_iterator(&MI));
+         It != MI.getParent()->rend(); It++) {
+      const MachineInstr *CurrMI = &*It;
+      if (CurrMI->modifiesRegister(RegUsedInAddr, TRI)) {
+        ModifyingMI = const_cast<MachineInstr *>(CurrMI);
+        break;
+      }
+    }
+    if (!ModifyingMI)
+      return false;
+    // Check for the const value defined in register by ModifyingMI. This means
+    // all other previous values for that register has been invalidated.
+    int64_t ImmVal;
+    if (!TII->getConstValDefinedInReg(*ModifyingMI, RegUsedInAddr, ImmVal))
+      return false;
+    // Calculate the reg size in bits, since this is needed for bailing out in
+    // case of overflow.
+    int32_t RegSizeInBits = TRI->getRegSizeInBits(RegUsedInAddr, MRI);
+    APInt ImmValC(RegSizeInBits, ImmVal, true /*IsSigned*/);
+    APInt MultiplierC(RegSizeInBits, Multiplier);
+    assert(MultiplierC.isStrictlyPositive() &&
+           "expected to be a positive value!");
+    bool IsOverflow;
+    // Sign of the product depends on the sign of the ImmVal, since Multiplier
+    // is always positive.
+    APInt Product = ImmValC.smul_ov(MultiplierC, IsOverflow);
+    if (IsOverflow)
+      return false;
+    APInt DisplacementC(64, Displacement, true /*isSigned*/);
+    DisplacementC = Product.sadd_ov(DisplacementC, IsOverflow);
+    if (IsOverflow)
+      return false;
+
+    // We only handle diplacements upto 64 bits wide.
+    if (DisplacementC.getActiveBits() > 64)
+      return false;
+    Displacement = DisplacementC.getSExtValue();
+    return true;
+  };
+
+  // If a register used in the address is constant, fold it's effect into the
+  // displacement for ease of analysis.
+  bool BaseRegIsConstVal = false, ScaledRegIsConstVal = false;
+  if (CalculateDisplacementFromAddrMode(BaseReg, 1))
+    BaseRegIsConstVal = true;
+  if (CalculateDisplacementFromAddrMode(ScaledReg, AddrMode.Scale))
+    ScaledRegIsConstVal = true;
+
+  // The register which is not null checked should be part of the Displacement
+  // calculation, otherwise we do not know whether the Displacement is made up
+  // by some symbolic values.
+  // This matters because we do not want to incorrectly assume that load from
+  // falls in the zeroth faulting page in the "sane offset check" below.
+  if ((BaseReg && BaseReg != PointerReg && !BaseRegIsConstVal) ||
+      (ScaledReg && ScaledReg != PointerReg && !ScaledRegIsConstVal))
     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 (!(-PageSize < Offset && Offset < PageSize))
+  if (!(-PageSize < Displacement && Displacement < PageSize))
     return SR_Unsuitable;
 
   // Finally, check whether the current memory access aliases with previous one.
   for (auto *PrevMI : PrevInsts) {
     AliasResult AR = areMemoryOpsAliased(MI, PrevMI);
     if (AR == AR_WillAliasEverything)
       return SR_Impossible;
     if (AR == AR_MayAlias)
       return SR_Unsuitable;
   }

llvm/lib/Target/AArch64/AArch64InstrInfo.h

public:
   static unsigned convertToFlagSettingOpc(unsigned Opc, bool &Is64Bit);
 
   /// Return true if this is a load/store that can be potentially paired/merged.
   bool isCandidateToMergeOrPair(const MachineInstr &MI) const;
 
   /// Hint that pairing the given load or store is unprofitable.
   static void suppressLdStPair(MachineInstr &MI);
 
+  Optional<ExtAddrMode>
+  getAddrModeFromMemoryOp(const MachineInstr &MemI,
+                          const TargetRegisterInfo *TRI) const override;
+
   bool getMemOperandsWithOffsetWidth(
       const MachineInstr &MI, SmallVectorImpl<const MachineOperand *> &BaseOps,
       int64_t &Offset, bool &OffsetIsScalable, unsigned &Width,
       const TargetRegisterInfo *TRI) const override;
 
   /// If \p OffsetIsScalable is set to 'true', the offset is scaled by `vscale`.
   /// This is true for some SVE instructions like ldr/str that have a
   /// 'reg + imm' addressing mode where the immediate is an index to the

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

bool AArch64InstrInfo::getMemOperandsWithOffsetWidth(
   const MachineOperand *BaseOp;
   if (!getMemOperandWithOffsetWidth(LdSt, BaseOp, Offset, OffsetIsScalable,
                                     Width, TRI))
     return false;
   BaseOps.push_back(BaseOp);
   return true;
 }
 
+Optional<ExtAddrMode>
+AArch64InstrInfo::getAddrModeFromMemoryOp(const MachineInstr &MemI,
+                                          const TargetRegisterInfo *TRI) const {
+  const MachineOperand *Base; // Filled with the base operand of MI.
+  int64_t Offset;             // Filled with the offset of MI.
+  bool OffsetIsScalable;
+  if (!getMemOperandWithOffset(MemI, Base, Offset, OffsetIsScalable, TRI))
+    return None;
+
+  if (!Base->isReg())
+    return None;
+  ExtAddrMode AM;
+  AM.BaseReg = Base->getReg();
+  AM.Displacement = Offset;
+  AM.ScaledReg = 0;
+  return AM;
+}
+
 bool AArch64InstrInfo::getMemOperandWithOffsetWidth(
     const MachineInstr &LdSt, const MachineOperand *&BaseOp, int64_t &Offset,
     bool &OffsetIsScalable, unsigned &Width,
     const TargetRegisterInfo *TRI) const {
   assert(LdSt.mayLoadOrStore() && "Expected a memory operation.");
   // Handle only loads/stores with base register followed by immediate offset.
   if (LdSt.getNumExplicitOperands() == 3) {
     // Non-paired instruction (e.g., ldr x1, [x0, #8]).

llvm/lib/Target/X86/X86InstrInfo.h

void replaceBranchWithTailCall(MachineBasicBlock &MBB,
                                  SmallVectorImpl<MachineOperand> &Cond,
                                  const MachineInstr &TailCall) const override;
 
   bool analyzeBranch(MachineBasicBlock &MBB, MachineBasicBlock *&TBB,
                      MachineBasicBlock *&FBB,
                      SmallVectorImpl<MachineOperand> &Cond,
                      bool AllowModify) const override;
 
+  Optional<ExtAddrMode>
+  getAddrModeFromMemoryOp(const MachineInstr &MemI,
+                          const TargetRegisterInfo *TRI) const override;
+
+  bool getConstValDefinedInReg(const MachineInstr &MI, const Register Reg,
+                               int64_t &ImmVal) const override;
+
   bool preservesZeroValueInReg(const MachineInstr *MI,
                                const Register NullValueReg,
                                const TargetRegisterInfo *TRI) const override;
 
   bool getMemOperandsWithOffsetWidth(
       const MachineInstr &LdSt,
       SmallVectorImpl<const MachineOperand *> &BaseOps, int64_t &Offset,
       bool &OffsetIsScalable, unsigned &Width,

llvm/lib/Target/X86/X86InstrInfo.cpp

case 64:
     assert(STI.hasAVX512() && "Using 512-bit register requires AVX512");
     if (isStackAligned)
       return load ? X86::VMOVAPSZrm : X86::VMOVAPSZmr;
     else
       return load ? X86::VMOVUPSZrm : X86::VMOVUPSZmr;
   }
 }
 
+Optional<ExtAddrMode>
+X86InstrInfo::getAddrModeFromMemoryOp(const MachineInstr &MemI,
+                                      const TargetRegisterInfo *TRI) const {
+  const MCInstrDesc &Desc = MemI.getDesc();
+  int MemRefBegin = X86II::getMemoryOperandNo(Desc.TSFlags);
+  if (MemRefBegin < 0)
+    return None;
+
+  MemRefBegin += X86II::getOperandBias(Desc);
+
+  auto &BaseOp = MemI.getOperand(MemRefBegin + X86::AddrBaseReg);
+  if (!BaseOp.isReg()) // Can be an MO_FrameIndex
+    return None;
+
+  const MachineOperand &DispMO = MemI.getOperand(MemRefBegin + X86::AddrDisp);
+  // Displacement can be symbolic
+  if (!DispMO.isImm())
+    return None;
+
+  ExtAddrMode AM;
+  AM.BaseReg = BaseOp.getReg();
+  AM.ScaledReg = MemI.getOperand(MemRefBegin + X86::AddrIndexReg).getReg();
+  AM.Scale = MemI.getOperand(MemRefBegin + X86::AddrScaleAmt).getImm();
+  AM.Displacement = DispMO.getImm();
+  return AM;
+}
+
+bool X86InstrInfo::getConstValDefinedInReg(const MachineInstr &MI,
+                                           const Register Reg,
+                                           int64_t &ImmVal) const {
+  if (MI.getOpcode() != X86::MOV32ri && MI.getOpcode() != X86::MOV64ri)
+    return false;
+  // Mov Src can be a global address.
+  if (!MI.getOperand(1).isImm() || MI.getOperand(0).getReg() != Reg)
+    return false;
+  ImmVal = MI.getOperand(1).getImm();
+  return true;
+}
+
 bool X86InstrInfo::preservesZeroValueInReg(
     const MachineInstr *MI, const Register NullValueReg,
     const TargetRegisterInfo *TRI) const {
   if (!MI->modifiesRegister(NullValueReg, TRI))
     return true;
   switch (MI->getOpcode()) {
   // Shift right/left of a null unto itself is still a null, i.e. rax = shl rax
   // X.

llvm/test/CodeGen/X86/implicit-null-check-negative.ll

define i64 @imp_null_check_load_shift_add_addr(i64* %x, i64 %r) {
    %y = ptrtoint i64* %x to i64
    %shry = shl i64 %y, 6
    %shry.add = add i64 %shry, %r
    %y.ptr = inttoptr i64 %shry.add to i64*
    %x.loc = getelementptr i64, i64* %y.ptr, i64 1
    %t = load i64, i64* %x.loc
    ret i64 %t
 }
+
+; the memory op is not within faulting page.
+define i64 @imp_null_check_load_addr_outside_faulting_page(i64* %x) {
+  entry:
+   %c = icmp eq i64* %x, null
+   br i1 %c, label %is_null, label %not_null, !make.implicit !0
+
+  is_null:
+   ret i64 42
+
+  not_null:
+   %y = ptrtoint i64* %x to i64
+   %shry = shl i64 %y, 3
+   %shry.add = add i64 %shry, 68719472640
+   %y.ptr = inttoptr i64 %shry.add to i64*
+   %x.loc = getelementptr i64, i64* %y.ptr, i64 1
+   %t = load i64, i64* %x.loc
+   ret i64 %t
+}
+
 !0 = !{}

llvm/test/CodeGen/X86/implicit-null-check.ll

-; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -verify-machineinstrs -O3 -mtriple=x86_64-apple-macosx -enable-implicit-null-checks < %s | FileCheck %s
 
 define i32 @imp_null_check_load(i32* %x) {
 ; CHECK-LABEL: imp_null_check_load:
 ; CHECK:       ## %bb.0: ## %entry
 ; CHECK-NEXT:  Ltmp0:
 ; CHECK-NEXT:    movl (%rdi), %eax ## on-fault: LBB0_1
 ; CHECK-NEXT:  ## %bb.2: ## %not_null
; CHECK-NEXT:    retq
    %y.ptr = inttoptr i64 %shry to i64*
    %x.loc = getelementptr i64, i64* %y.ptr, i64 1
    %t = load i64, i64* %x.loc
    ret i64 %t
 }
 
 ; Same as imp_null_check_load_shift_addr but shift is by 3 and this is now
 ; converted into complex addressing.
-; TODO: Can be converted into implicit null check
 define i64 @imp_null_check_load_shift_by_3_addr(i64* %x) {
 ; CHECK-LABEL: imp_null_check_load_shift_by_3_addr:
 ; CHECK:       ## %bb.0: ## %entry
-; CHECK-NEXT:    testq %rdi, %rdi
-; CHECK-NEXT:    je LBB22_1
+; CHECK-NEXT:  Ltmp18:
+; CHECK-NEXT:    movq 8(,%rdi,8), %rax ## on-fault: LBB22_1
 ; CHECK-NEXT:  ## %bb.2: ## %not_null
-; CHECK-NEXT:    movq 8(,%rdi,8), %rax
 ; CHECK-NEXT:    retq
 ; CHECK-NEXT:  LBB22_1: ## %is_null
 ; CHECK-NEXT:    movl $42, %eax
 ; CHECK-NEXT:    retq
 
   entry:
    %c = icmp eq i64* %x, null
    br i1 %c, label %is_null, label %not_null, !make.implicit !0
 
   is_null:
    ret i64 42
 
   not_null:
    %y = ptrtoint i64* %x to i64
    %shry = shl i64 %y, 3
    %y.ptr = inttoptr i64 %shry to i64*
    %x.loc = getelementptr i64, i64* %y.ptr, i64 1
    %t = load i64, i64* %x.loc
    ret i64 %t
 }
+
+define i64 @imp_null_check_load_shift_add_addr(i64* %x) {
+; CHECK-LABEL: imp_null_check_load_shift_add_addr:
+; CHECK:       ## %bb.0: ## %entry
+; CHECK:         movq 3526(,%rdi,8), %rax ## on-fault: LBB23_1
+; CHECK-NEXT:  ## %bb.2: ## %not_null
+; CHECK-NEXT:    retq
+; CHECK-NEXT:  LBB23_1: ## %is_null
+; CHECK-NEXT:    movl $42, %eax
+; CHECK-NEXT:    retq
+
+  entry:
+   %c = icmp eq i64* %x, null
+   br i1 %c, label %is_null, label %not_null, !make.implicit !0
+
+  is_null:
+   ret i64 42
+
+  not_null:
+   %y = ptrtoint i64* %x to i64
+   %shry = shl i64 %y, 3
+   %shry.add = add i64 %shry, 3518
+   %y.ptr = inttoptr i64 %shry.add to i64*
+   %x.loc = getelementptr i64, i64* %y.ptr, i64 1
+   %t = load i64, i64* %x.loc
+   ret i64 %t
+}
 !0 = !{}

llvm/test/CodeGen/X86/implicit-null-checks.mir

--- |
 
   not_null:
     ret i32 undef
 
   is_null:
     ret i32 undef
   }
 
+  define i32 @imp_null_check_address_mul_overflow(i32* %x, i32 %a) {
+  entry:
+    %c = icmp eq i32* %x, null
+    br i1 %c, label %is_null, label %not_null, !make.implicit !0
+  
+  is_null:                                          ; preds = %entry
+    ret i32 42
+  
+  not_null:                                         ; preds = %entry
+    %y = ptrtoint i32* %x to i32
+    %y64 = zext i32 %y to i64
+    %b = mul i64 %y64, 9223372036854775807 ; 0X0FFFF.. i.e. 2^63 - 1
+    %z = trunc i64 %b to i32
+    ret i32 %z
+  }
+
   attributes #0 = { "target-features"="+bmi,+bmi2" }
 
   !0 = !{}
 ...
 ---
 name:            imp_null_check_with_bitwise_op_0
 # CHECK-LABEL: name:            imp_null_check_with_bitwise_op_0
 alignment:       16
bb.1.not_null:
     $eax = MOV32rr $edi
     RETQ $eax
 
   bb.2.is_null:
     $eax = XOR32rr undef $eax, undef $eax, implicit-def dead $eflags
     RETQ $eax
 
 ...
+---
+name:            imp_null_check_address_mul_overflow
+# CHECK-LABEL: name:            imp_null_check_address_mul_overflow
+# CHECK:  bb.0.entry:
+# CHECK-NOT: FAULTING_OP
+alignment:       16
+tracksRegLiveness: true
+liveins:         
+  - { reg: '$rdi' }
+  - { reg: '$rsi' }
+body:             |
+  bb.0.entry:
+    liveins: $rsi, $rdi
+  
+    TEST64rr $rdi, $rdi, implicit-def $eflags
+    JCC_1 %bb.1, 4, implicit $eflags
+  
+  bb.2.not_null:
+    liveins: $rdi, $rsi
+  
+    $rcx = MOV64ri -9223372036854775808
+    $eax = MOV32rm killed $rdi, 2, $rcx, 0, $noreg, implicit-def $rax
+    RETQ $eax
+  
+  bb.1.is_null:
+    $eax = MOV32ri 42
+    RETQ $eax
+
+...