[PATCH] D34207: [IndVarSimplify] Add AShr exact flags using induction variables ranges.

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Hello, Thanks for the review. It sounds like you are happy with where this is, so I will upload a cleaned up version. Thanks for the pointers.

Unfortunately it looks like something else I need for this has broken over in r305481 :(  I'll have to try and investigate that to see exactly why it's stopping the full case from optimising.

To this, I've added LShr's, as well as AShr's and the extra testing. Let me know if anything else needs changing, but no need to hurry as I need to look into that other thing now too.



================
Comment at: lib/Transforms/Utils/SimplifyIndVar.cpp:542
+      if(ShlOp1 == IVOperand || !BO->getType()->isIntegerTy() ||
+          IVRange.getUpper().uge(BO->getType()->getPrimitiveSizeInBits()))
+        continue;
----------------
sanjoy wrote:
> Do you really mean `getUpper` or do you want `getUnsignedMax`?
> 
> Secondly, `BO` will always be an integer (as opposed to a vector of integers); since the induction variable is an integer.
> 
> Thirdly, if I'm not missing something, the `IVRange.getUpper().uge(BO->getType()->getPrimitiveSizeInBits())` check should be unnecessary.  If the IV is larger than the bitwidth then `BO` is poison, and so is `AShr` (and making it exact does not make it any more poison).
This is my understanding, yes. But I thought the same for shifts in computeKnownBits, so I thought it best to be careful. In the case for the original test it was known what the upper bound for the loop is. Removing the need for this will only make this fire for more loops, which should be good.


================
Comment at: test/Transforms/IndVarSimplify/strengthen-overflow.ll:123
+  %0 = load i32, i32* %arrayidx, align 4
+  %1 = load i32, i32* %arrayidx1, align 4
+  %sub = sub nsw i32 %1, %0
----------------
sanjoy wrote:
> I think most of the load/store stuff here can be pruned.
Sounds good. This was a bit strategic, in case I had to explain where the benefits come in ;)


https://reviews.llvm.org/D34207