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

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:390
+                                               int64_t Multiplier) {
+    if (!RegUsedInAddr || !Multiplier)
+      return;
----------------
reames wrote:
> These should be impossible, use asserts.
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.


================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:408
+      return;
+    Displacement += ImmVal * Multiplier;
+    if (RegUsedInAddr == BaseReg)
----------------
reames wrote:
> 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.
good catch! will take a look.


================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:412
+    else
+      ScaledRegIsConstVal = true;
+  };
----------------
reames wrote:
> Please return true or false, then set the per register flag.  This is too much coupling.
yes, agreed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87148