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

[Peter Collingbourne via llvm-commits]

pcc added inline comments.

================
Comment at: docs/LangRef.rst:7548
@@ +7547,3 @@
+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
----------------
hfinkel wrote:
> eli.friedman wrote:
> > Hmm... this sort of restricts the possible uses of the marking.  For example, given:
> > 
> >     struct X {
> >         int x, y[2];
> >     };
> >     int f(struct X *x, int i) { return x->y[i]; }
> >     int *f2(struct X *x) { return x->y; }
> > 
> > We can mark the GEP in `x->y[i]` inrange, but we can't mark the GEP in `x->y` inrange because it could be used in a pointer comparison.  I guess that's okay (the stated form is convenient for performing SROA).  That said, inevitably someone is going to push for a version which doesn't make comparisons undefined so it can be used more aggressively in C code.
> > 
> > You should probably state explicitly that the result of converting an inrange pointer to an integer is undef; you can't actually do anything useful with the result anyway.
> What's the motivation for the current language? I don't think we can have anything that strict.
The intent was to allow optimization passes to re-arrange elements of an indexed object (i.e. what SplitGlobals does). Although at this point it does seem that trying to restrict pointer comparisons in general would be likely to make this too restrictive for use by C family frontends. Instead I think the best approach would be to change SplitGlobals to check for the unnamed_addr attribute, and remove the pointer comparison language here.

> You should probably state explicitly that the result of converting an inrange pointer to an integer is undef;

Will do.

================
Comment at: docs/LangRef.rst:7550
@@ +7549,3 @@
+a GEP which was evaluated with identical operands up to and including the
+``inrange`` operand. Note that the ``inrange`` keyword is currently only
+allowed in constant expressions.
----------------
sanjoy wrote:
> sanjoy wrote:
> > This is a little odd since (IIUC) it implies that replacing an `inrange` GEP with a not `inrange` version of the same GEP can introduce undefined behavior; that is:
> > 
> > ```
> > x0 = gep_inrange(A, 42)
> > x1 = gep_normal(A, 42)
> > result = x0 == x0 ;; perhaps hidden behind a function
> > ```
> > 
> > to
> > 
> > ```
> > x0 = gep_inrange(A, 42)
> > x1 = gep_normal(A, 42)
> > result = x0 == y0 ;; perhaps hidden behind a function
> > ```
> > 
> > via x1->replaceAllUsesWith(x0).
> > 
> > Why do you need this property / caveat?
> There's a typo, in the transformed snippet we should have `result = x0 == x1`.  Also pretend that there are other uses of `x1` (so it isn't trivially DCE'ed away).
I think this is moot in light of my other comment.

================
Comment at: docs/LangRef.rst:7551
@@ +7550,3 @@
+``inrange`` operand. Note that the ``inrange`` keyword is currently only
+allowed in constant expressions.
+
----------------
sanjoy wrote:
> hfinkel wrote:
> > pcc wrote:
> > > hfinkel wrote:
> > > > pcc wrote:
> > > > > 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.
> > > > Aside from the LangRef, what needs to change to allow non-constant indicies? I think they should be allowed, even if taking advantage of them is future work. I don't see anything in the parser changes that forbids them.
> > > We would need printing and parsing support, as well as a bitcode representation for getelementptr instructions (as opposed to only constants) with an inrange annotation. Should be straightforward, but a non-zero amount of work.
> > Ah, so it only applies to constants right now, not to actual GEP instructions. I see what you mean now, but when I originally read the text, I thought that you meant that the index expression needed to be constant, not that the entire GEP needed to be a constant.
> Given that, it is probably better to say "allowed in constant getelementptr expressions"
Will do


https://reviews.llvm.org/D22793