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

[Eli Friedman via llvm-commits]

efriedma added a comment.

I haven't reviewed the assembly and bitcode reading/writing changes.

The semantics here look right.



================
Comment at: llvm/docs/LangRef.rst:7578
+element selected by the index marked as ``inrange``. The result of a pointer
+comparison or ``ptrtoint`` involving a pointer derived from a ``getelementptr``
+with the ``inrange`` keyword is undefined, with the exception of comparisons
----------------
Might want to clarify that this includes ptrtoint-like operations involving memory.


================
Comment at: llvm/include/llvm/IR/Operator.h:386
+    if (SubclassOptionalData >> 1 == 0) return None;
+    return (SubclassOptionalData >> 1) - 1;
+  }
----------------
Please add a comment somewhere describing exactly how many bits are dedicated to InRangeIndex.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:950
+  Constant *C = ConstantExpr::getGetElementPtr(
+      SrcElemTy, Ptr, NewIdxs, /*InBounds=*/false, InRangeIndex);
   assert(C->getType()->getPointerElementType() == Ty &&
----------------
Should we preserve inbounds here?  (Please add a FIXME if it's complicated somehow.)


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:970
 /// information, due to only being passed an opcode and operands. Constant
 /// folding using this function strips this information.
 ///
----------------
Does this patch affect this TODO somehow?


================
Comment at: llvm/lib/IR/ConstantFold.cpp:551
+               // information.
+               !cast<GEPOperator>(CE)->getInRangeIndex().hasValue()) {
       // If all of the indexes in the GEP are null values, there is no pointer
----------------
This is a little awkward... but I don't really see an alternative; it's okay, I guess.


https://reviews.llvm.org/D22793