[PATCH] D143437: [llvm] Use pointer index type for more GEP offsets (pre-codegen)

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/NaryReassociate.cpp:454
+  if (RHS->getType() != PtrIdxTy)
+    RHS = Builder.CreateSExtOrTrunc(RHS, PtrIdxTy);
   if (IndexedSize != ElementSize) {
----------------
jrtc27 wrote:
> krzysz00 wrote:
> > foad wrote:
> > > Not necessarily related to your patch, but the "OrTrunc" makes me nervous (here and in a couple of other places in this patch). My understanding of the index type in datalayout is that it is a preferred type to use GEP indexes, but shouldn't actually have any semantic effect, so it would be wrong to truncate an index to the index type unless you can prove that it won't change the value.
> > That's not my understanding of that field. The index field is maximal size of an offset into a pointer in address space N. For example, CHERI's capability pointers are (to my understanding) `128:128:128:64` meaning that the pointer value is 128 bits long, but only 64 bits of offset can be applied to it.
> > 
> > That is, in C terms, the pointer type is `intptr_t` and the index field is `size_t`. This patch fixes up a bunch of code that normalized pointer offsets to `intptr_t` instead of `size_t`, since LLVM historically assumed they were the same.
> This is correct; see SelectionDAGBuilder::visitGetElementPtr for example where the use of the index type is explicit (and there's some commentary, too). It is legal to use an oversized type in the IR, but it will eventually be legalised. Undersized types are implicitly sign-extended.
If it has a semantic effect on IR then that needs to be clearly documented in the LangRef. Literally the only mention of it in the document is: "The fourth parameter `<idx>` is a size of index that used for address calculation."


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143437