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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

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



================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:980
 
+  /// Returns true if MI is instruction that defines Reg to have a constant
+  /// value and the value is recorded in ImmVal.
----------------
"is an instruction"


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


================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:408
+      return;
+    Displacement += ImmVal * Multiplier;
+    if (RegUsedInAddr == BaseReg)
----------------
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.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87148