[PATCH] [SeparateConstOffsetFromGEP] Fix bugs and improve the current SeparateConstOffsetFromGEP

[Jingyue Wu]

I think this 1+2 statement should be optimized away regardless of SeparateConstOffset, because leaving such misoptimization can hurt other passes down the road. This can be done by either running instsimplify after simplifycfg or fixing simplifycfg so that it simplifies the new instructions it creates. It's worth a separate bug report. Btw, I tried simple.c on the NVPTX backend, and found 1+2 is produced by CodeGenPrepare instead of simplifycfg. We probably need to fix the NVPTX backend similarly. 

Handling "((a << 2) + 12) | 1" should probably be a feature request to instcombine, because doing so benefits other passes and the code quality in general. 

These are all very nice finds, and I like them! I just think they should be fixed in a more general way. 

Regarding the improvement on being able to extract multiple constants, I think your implementation is correct in general and I really appreciate that you figured it out. However, I am concerned about the complexity of the code. First, with the above issues in instcombine and simplifycfg fixed, are we still motivated in handling multiple constant offsets in this pass? Second, if we are still motivated, instead of finding and extracting all non-zero constant offsets in one DFS traversal, can we repeatedly call find+extract until we cannot improve any more? Your current implementation is surely faster, but I suspect (1) most GEPs have no constant offsets, and (2) for those who have, the latter approach would only run a very limited number (mostly two) of iterations. In general, I'd trade premature optimization for code simplicity. 

Let me know what you think. Thanks!

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:129
@@ -130,2 +128,3 @@
   /// Looks for a constant offset without extracting it. The meaning of the
   /// arguments and the return value are the same as Extract.
+  static int64_t Find(Value *Idx, const DataLayout *DL, GetElementPtrInst *GEP,
----------------
HaoLiu wrote:
> jingyue wrote:
> > Not the same as Extract any more. 
> I just think we don't need to return an int64_t, which is only used to check whether the extraction is successful. As we always need to return a NewIdx, we can check whether the NewIdx is nullptr.
Yes, I agree and I like that. I was just saying the comments need to be updated :)

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:739
@@ -685,1 +738,3 @@
   GEP->setIsInBounds(false);
+  // No need to create another GEP as the accumulative byte offset is 0.
+  if (AccumulativeByteOffset == 0)
----------------
HaoLiu wrote:
> jingyue wrote:
> > Not quite true. The sum of offsets being 0 doesn't mean every offset is zero. As long as there's a non-zero offset, it's worth creating a new GEP. 
> Sorry I still not quite understand. This is after setting new operands stage, which means the old GEP has already been replaced by new indices. The following code is only to create the second GEP with one index whose value is 0. So I still think a GEP with index 0 is unnecessary.
Sounds good.

Nits: change the comment s/as/if, and add a blank line after GEP->setIsInBoiunds(false). Thanks!

http://reviews.llvm.org/D5863