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

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added a comment.

Thank you very much for working on this. It will make our future upstream merges much easier.



================
Comment at: ../docs/LangRef.rst:1913
     This specifies the *size* of a pointer and its ``<abi>`` and
-    ``<pref>``\erred alignments for address space ``n``. All sizes are in
-    bits. The address space, ``n``, is optional, and if not specified,
+    ``<pref>``\erred alignments for address space ``n``. The forth parameter
+    ``<idx>`` is a size of index that used for address calculation. If not
----------------
fourth parameter


================
Comment at: ../include/llvm/IR/DataLayout.h:357
+    return getIndexSize(AS) * 8;
+  }
+
----------------
theraven wrote:
> Please can we not have a default for AS?  We've added defaults for other things like this because they were existing APIs and we didn't want to have to update all of the callers at once, but all of the callers of this are already being updated and so should specify the correct AS.
Yes, please remove the default value here. We have run into lots of issues due to using the size of AS0 instead of the correct one.


================
Comment at: ../lib/Transforms/InstCombine/InstructionCombining.cpp:1511
+
+  // Index width shouldn't have the same width as pointer. Data layout chooses
+  // the right type based on supported integer types.
----------------
Index width may not be the same width as pointer width


Repository:
  rL LLVM

https://reviews.llvm.org/D42123