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

[Peter Collingbourne via llvm-commits]

pcc added a comment.

> Does the way you're using subclass data mean that GVN just works correctly without any additional changes? Or does it need to be modified to avoid unifying a non-inrange GEP with an inrange GEP?


If we allow this attribute on instructions, we would need to modify `andIRFlags` to recognize the new attribute, as David mentioned. At the moment, we only allow it on constants (I will document this more clearly).

> inrange markings probably interact badly with certain existing optimizations; for example, a gep with all zero indexes plus a bitcast currently gets folded into just a bitcast, but that loses the inrange marking.


That specific fold is one that I inhibited, see lib/IR/ConstantFold.cpp, line 551.

> Not a correctness problem, but it'll probably be difficult to fix the resulting missed optimizations.


While working on this patch I did try to identify any missed optimizations specific to vtables by hacking in some code like this:

  diff --git a/lib/IR/Constants.cpp b/lib/IR/Constants.cpp
  index 1fbd75f..1d445ff 100644
  --- a/lib/IR/Constants.cpp
  +++ b/lib/IR/Constants.cpp
  @@ -1940,6 +1940,13 @@ Constant *ConstantExpr::getGetElementPtr(Type *Ty, Constant *C,
       ArgVec.push_back(Idx);
     }
   
  +#if 0
  +  if (isa<GlobalVariable>(C) && C->getName().startswith("_ZTV") &&
  +      C->getName().find("type_info") == StringRef::npos &&
  +      (!InBoundsIndex || *InBoundsIndex != 1))
  +    abort();
  +#endif
  +
     unsigned SubClassOptionalData = InBounds ? GEPOperator::IsInBounds : 0;
     if (InBoundsIndex && *InBoundsIndex < 63)
       SubClassOptionalData |= (*InBoundsIndex + 1) << 1;

The constant folding changes I made in this patch were sufficient to be able to compile Chromium on Linux without aborting (with the above code turned on) and without a regression in binary size.

I'm not sure if there's a better way in general to track missed optimizations.

> That's more of a question of what makes sense for clang rather than something which affects the attribute itself... but if clang can't really use inrange markings outside of vtables, that reduces the utility a lot.


You have a point there, but I'd expect that frontends for more memory safe languages may be able to make more extensive use of this attribute than clang.


https://reviews.llvm.org/D22793