[PATCH] D87148: [ImplicitNullCheck] Handle Nonzero faulting pages and complex addressing

Philip Reames via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Sep 29 11:41:32 PDT 2020


reames accepted this revision.
reames added a comment.
This revision is now accepted and ready to land.

LGTM, but with required changes.



================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:384
+  if (BaseReg != PointerReg && ScaledReg != PointerReg)
     return SR_Unsuitable;
 
----------------
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.  


================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:416
+    int32_t RegSizeInBits = TRI->getRegSizeInBits(RegUsedInAddr, MRI);
+    const bool IsSignedImmVal = ImmVal < 0;
+    APInt ImmValC(RegSizeInBits, ImmVal, IsSignedImmVal);
----------------
Replace IsSignedImmVal with constant true.

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


================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:418
+    APInt ImmValC(RegSizeInBits, ImmVal, IsSignedImmVal);
+    APInt Product(RegSizeInBits, 0);
+    APInt MultiplierC(RegSizeInBits, Multiplier);
----------------
You should be able to just define the APInt in the definition below.  The bitwidth will come from the RHS.


================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:425
+    // is always positive.
+    Product = IsSignedImmVal ? ImmValC.smul_ov(MultiplierC, IsOverflow)
+                             : ImmValC.umul_ov(MultiplierC, IsOverflow);
----------------
e,g, APInt Product = 

Also, remember to constant fold isSignedImmVal


================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:437
+    // We only handle diplacements upto 64 bits wide.
+    if (DisplacementC.getActiveBits() > 64)
+      return false;
----------------
You could if you wished do Displacement mod 2^64, but I don't see any point in that.


================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:443
+
+  bool BaseRegIsConstVal = false, ScaledRegIsConstVal = false;
+  if (CalculateDisplacementFromAddrMode(BaseReg, 1))
----------------
Add comment here: "If a register used in the address is constant, fold it's effect into the displacement for ease of analysis."


================
Comment at: llvm/test/CodeGen/X86/implicit-null-check.ll:625
+; CHECK:       ## %bb.0: ## %entry
+; CHECK:         movq 3526(,%rdi,8), %rax ## on-fault: LBB23_1
+; CHECK-NEXT:  ## %bb.2: ## %not_null
----------------
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)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87148/new/

https://reviews.llvm.org/D87148



More information about the llvm-commits mailing list