[PATCH] D53162: [DataLayout] Add bit width of pointers to global values

[David Chisnall via Phabricator via llvm-commits]

theraven added inline comments.


================
Comment at: include/llvm/IR/DataLayout.h:340
+  /// to be 0.
+  unsigned getPointerGlobalBitWidth(unsigned AS = 0) const;
+
----------------
bjope wrote:
> Maybe remove the default here. There are comments about removing AS defaults for other methods here, and usually when someone does not provide the address space in the call it could be a hard to find fault.
Agreed.  The default value is useful for incrementally migrating consumers of APIs that didn't know about address spaces, but for a new API it doesn't make sense.


================
Comment at: lib/Analysis/ValueTracking.cpp:1344
+          if (LeadZ > 0)
+            LeadZ--;
+        }
----------------
Does this correctly handle the case of invalid pointers being created as temporary values?  In CHERI, we saw a few cases where, after reassociation, you ended up with IR that took a pointer out of bounds in one calculation and then brought it back in as a result of the next one before dereferencing.  This should perhaps be restricted to inbounds GEPs (or is it already and I'm missing the check)?


================
Comment at: lib/IR/DataLayout.cpp:311
+      // Size of global value pointers.
+      unsigned GlobalBitWidth = 8 * PointerMemSize;
+
----------------
Please can we have a comment that we're assuming 8-bit bytes here?  A few out-of-tree targets are trying to make things work with different addressable sizes and it's polite to leave them a signpost that this is a thing that they'll need to change.


Repository:
  rL LLVM

https://reviews.llvm.org/D53162