[PATCH] D42123: Derive GEP index type from Data Layout

[David Chisnall via Phabricator via llvm-commits]

theraven added inline comments.


================
Comment at: ../lib/Analysis/ScalarEvolution.cpp:3675
   assert(isSCEVable(Ty) && "Type is not SCEVable!");
+  if (Ty->isPointerTy())
+    return getDataLayout().getIndexTypeSizeInBits(Ty);
----------------
sanjoy wrote:
> theraven wrote:
> > theraven wrote:
> > > Ayal wrote:
> > > > delena wrote:
> > > > > sanjoy wrote:
> > > > > > I don't think this is a correct place to make this change -- the size of a pointer is the size of a pointer.  I think you need to change the SCEV corresponding to GEP(Ptr, Idx) to be "sext(Ptr) + Idx" or "Ptr + sext(Idx)" depending on their relative sizes.
> > > > > I can't create SCEV expressions with ptr+ind, it will fail with assertion on different types.
> > > > Elena, Sanjoy's thought above to change SCEV to be "sext(Ptr) + Idx" or "Ptr + sext(Idx)" will bring the two addends to be of the same type, i.e., of the larger type. The challenge in your case is lack of target support for integer addition of pointer-sized integers; which seems similar to CHERI's case. Except CHERI pointers (or capabilities) hold in addition to a standard-sized address additional information, such that the latter can be stripped out for SCEV purposes (IIUC - @theraven please correct if needed); whereas in your case the address itself is larger than a standard-sized integer. Perhaps for your case too the pointer can be stripped down to standard-sized integers to leverage SCEV's capabilities on "legal" types, which seems to be what your patch is doing, coupled with separate logic that deals with the stripped out bits(?).
> > > That's pretty much the case for us: our pointers are 128 bits, but have a 64-bit range (64 bits of metadata).  We have modified DataLayout to explicitly understand that pointer size and range are different (in a slightly hacky way, which we should improve before we think about upstreaming).  In scalar evolution, we always use the pointer's range as the type.  
> > > 
> > > We don't support arbitrary integer operations on pointers and in our back end we have added some new MVTs to represent non-integer pointer types.  Our architecture provides pointer + integer addressing modes.
> > > 
> > > I believe that, in the motivating example for this change, the existing ScalarEvolution code is correct: it should use pointer-sized integers, because otherwise the analyses are likely to be wrong in some exciting corner cases.  
> > > 
> > > We have addressed this by adding explicit PTRADD SelectionDAG nodes, which perform pointer + integer addition.  For complex addressing modes, we end up with (ptradd base (some complex integer ops)).  This works well as long as the underlying hardware supports address register + integer register addressing, which I presume is the case for Intel (it is for all Harvard architectures that I've come across).  
> > > 
> > > If you are targeting an architecture for which pointer operations and integer operations are not the same, then you should follow the same approach: in the back end, lower pointers to some non-integer type and match pointer operations with different patterns to integer ones.  We have a bunch of SelectionDAG and TableGen patches that make this work well, which we'd be happy to upstream.
> > I agree with @sanjoy that this isn't the correct place for this change, but it does happen to be the least disruptive place for the change.  The correct solution is probably to rename this method to something like `getTypeArithmeticSizeInBits` so that it's clear that it's returning a size as a proxy for a range and not a storage size.
> Given what you said, the right fix seems to be to truncate 128 bit pointers to 64 bits in getSCEV instead of lying about the pointer's size.  SCEV calls into other parts of LLVM like ValueTracking, and other parts of LLVM call into SCEV (invdvars, lsr, scev-aa etc.) and I'm worried that a discrepancy like this (pointer size = 64 in SCEV but 128 elsewhere) will cause bugs.
Truncating for us would be absolutely the wrong thing, because it changes the semantics (throws away all of the bounds metadata, falls back to some per-environment bounds which may not even allow access to this address).  In our case, we have a difference between the size and the range.  

All of the places we've seen this used in ScalarEvolution, it cares about the range, not the storage size, it just happens that on most architectures these are the same (because a 32-bit pointer has a range of 2^32 bytes, a 64-bit pointer has a range of 2^64 bytes).

The correct solution is to either rename this method something like `getTypeArithmeticSizeInBits` and update all of the callers, or add a new `getTypeArithmeticSizeInBits` method and update all of the callers (I don't believe that there are any that care about the storage size, but I might have missed one).

We've been running with a change that's semantically identical to @delena's for a few years and have not encountered any miscompilations as a result (building a whole of the FreeBSD base system, a bunch of benchmarks, and a few other large programs), so I'm fairly confident that it's safe.


Repository:
  rL LLVM

https://reviews.llvm.org/D42123