[PATCH] D22793: IR: Introduce inbounds attribute on getelementptr indices.

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: docs/LangRef.rst:7546-7549
@@ +7545,6 @@
+behavior if the load or store would access memory outside of the bounds of the
+element selected by the index marked as ``inrange``. The result of a pointer
+comparison is undefined if either operand was derived from an ``inrange``
+GEP, with the exception of comparisons where both operands were derived from
+a GEP which was evaluated with identical operands up to and including the
+``inrange`` operand. Note that the ``inrange`` keyword is currently only
----------------
majnemer wrote:
> What if I do a ptrtoint and compare the ints?
Yes, good point. "pointer comparison" can just become "comparison" then, I think.

================
Comment at: docs/LangRef.rst:7551
@@ +7550,3 @@
+``inrange`` operand. Note that the ``inrange`` keyword is currently only
+allowed in constant expressions.
+
----------------
hfinkel wrote:
> I don't think we should limit this to constant expressions. We should be able to use this to model p.x[i] vs. p.y. Removing the restriction to constant expressions will also mean you need to generalize the text about pointer comparisons: instead of saying that all operands must be identical, you'll need to say that the same elements need to be selected (or something like that).
Seems reasonable enough, but I'm not sure if I'll have any time soon to implement this for non-constants. Perhaps the non-constant implementation can be a separate patch.

================
Comment at: include/llvm/IR/Operator.h:382
@@ -381,1 +381,3 @@
   }
+  /// Returns the offset of the index with an inrange attachment, or None if
+  /// none.
----------------
hfinkel wrote:
> Can more than once index have inrange on it? It looks like no, but this is not clear from the LangRef, and I don't understand that restriction regardless.
No more than one. The rationale is that inrange on a given index will be at least as restrictive as inrange on any earlier index, so there's no point in allowing more than one. I'll document this in the langref.


https://reviews.llvm.org/D22793