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

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

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.



================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1283
+  /// calculation (even though they modify the base and index operands).
+  virtual bool getConstantAddrFromMemoryOp(
+      const MachineInstr &MI, int64_t &ConstantAddr, int64_t &Scale,
----------------
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.


================
Comment at: llvm/lib/CodeGen/ImplicitNullChecks.cpp:449
+                    [ConstantAddr](uint64_t StartAddress) {
+                      // Zeroth page is special cased when we have a negative
+                      // offset (greater than -PageSize), then the access will
----------------
>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.


================
Comment at: llvm/lib/Target/X86/X86InstrInfo.cpp:3701
+  // Returns true if we could safely update ConstantAddr.
+  auto UpdateAddrForComplexMode = [&](Register RegUsedInAddr,
+                                      int64_t multiplier) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87148