[PATCH] MergeFuncs should handle dynamic GEPs

[Pete Cooper]

Hi Stepan

>From LangRef (http://llvm.org/docs/LangRef.html#getelementptr-instruction), the interesting part for structures is that the GEP indices must be the same value.  If using a vector, it must be a splat of a constant.  I guess using a vector of the same constant is redundant (compared to a scalar) except that it keeps all the GEP indices matching in terms of all being vectors or all being scalar.  The part you want here is:

"When indexing into a (optionally packed) structure, only i32 integerconstants are allowed (when using a vector of indices they must all be the same i32 integer constant)”

For arrays or pointers, it says:

"When indexing into an array, pointer or vector, integers of any width are allowed, and they are not required to be constant”

The main new piece of functionality in MergeFuncs here is that for dynamic GEPs, it should be able to work out that 2 GEPs are equivalent even when the index is dynamic.  This is true when the underlying type has the same offset from a[i] to a[i + 1].  Eg, indexing a float[] vs int[] will give you the same offset for the same index ‘i’.  If any of this is unclear in the tests or patch comments, please let me know and i’ll be happy to improve them.


================
Comment at: test/Transforms/MergeFunc/gep.ll:63
@@ +62,3 @@
+  %bc_ptr = bitcast %vector_char4_ptr %this to %vector_char4_ptr
+  %offset_i = getelementptr %vector_char4_ptr %bc_ptr, <4 x i32><i32 0, i32 0, i32 0, i32 0>, <4 x i32><i32 1, i32 1, i32 1, i32 1>, <4 x i64> %i, <4 x i32><i32 2, i32 2, i32 2, i32 2>
+  %ext_i = extractelement <4 x i8*> %offset_i, i32 0
----------------
dyatkovskiy wrote:
> Dynamic GEP is quite new thing. Do we have specs for dynamic GEPs somewhere already? In particular rule definition for splat values?
> 
I think I answered this in the main comment at the end.  Let me know if anything else needs clarification.

================
Comment at: test/Transforms/MergeFunc/gep2.ll:54
@@ +53,3 @@
+  %bc_ptr = bitcast %vector_char4_ptr %this to %vector_char4_ptr
+  %offset_i = getelementptr %vector_char4_ptr %bc_ptr, <4 x i32><i32 0, i32 0, i32 0, i32 0>, <4 x i32><i32 1, i32 1, i32 1, i32 1>, <4 x i64> %i, <4 x i32><i32 0, i32 2, i32 2, i32 2>
+  %ext_i = extractelement <4 x i8*> %offset_i, i32 0
----------------
dyatkovskiy wrote:
> What would happen, if I'll create same method, but change this line to:
> %offset_i = getelementptr %vector_char4_ptr %bc_ptr, <4 x i32><i32 0, i32 0, i32 0, i32 0>, <4 x i32><i32 1, i32 1, i32 1, i32 1>, <4 x i64> %i, <4 x i32><i32 2, i32 2, i32 2, i32 0>
> It looks like your patch gonna merge such functions. Is it right?
Ah, you're totally right.  If you try to merge 2 GEPs, both with non-splat vectors, then both will give nullptr on these lines:

          ConstIntL = dyn_cast_or_null<ConstantInt>(ConstL->getSplatValue());
          ConstIntR = dyn_cast_or_null<ConstantInt>(ConstR->getSplatValue());

and then we'll get past this test, and crash

        if (int Res = cmpNumbers(!!ConstIntL, !!ConstIntR))
          return Res;

Nice catch!  I'll update the code and tests to handle this.

http://reviews.llvm.org/D7170

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/