[PATCH] D129958: [IndVars] Eliminate redundant type cast with different sizes

[Allen zhong via Phabricator via llvm-commits]

Allen marked 3 inline comments as done and an inline comment as not done.
Allen added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyIndVar.cpp:696
   if (MaskBits <= DestNumSigBits) {
-    for (User *U : UseInst->users()) {
+    for (User *U : make_early_inc_range(UseInst->users())) {
       // Match for fptosi/fptoui of sitofp and with same type.
----------------
nikic wrote:
> Why do we need an early inc range now? We're adding a new user of the IVOperand, not the UseInst, right?
I thought using make_early_inc_range would increase the robustness of the code referring to 85b4b21c8bb,  I'll delete it as it is surely unnecessary.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyIndVar.cpp:706
 
-      CI->replaceAllUsesWith(IVOperand);
+      PHINode *Phi = cast<PHINode>(IVOperand);
+      Value *Conv = nullptr;
----------------
nikic wrote:
> I don't think the IVOperand is necessarily a phi node. Can you add a test where the itofp is on the postinc IV (i.e. the add)?
I'm kind of curious, besides phi nodes, what other types of inductive variables can be expressed?

Sure, IVOperand does not need to be a phi node at this point.

PS: the new added case sitofp_fptosi_range_zext_postinc with postinc IV


================
Comment at: llvm/test/Transforms/IndVarSimplify/floating-point-small-iv.ll:138
+  %idxprom32 = sext i16 %conv to i32
+  %idxprom64 = sext i16 %conv to i64
+  %arrayidx = getelementptr inbounds [16777219 x i32], [16777219 x i32]* @array, i64 0, i64 %idxprom64
----------------
nikic wrote:
> I don't understand these tests. We don't want an existing sext user, but rather a wider type on the fptosi. So sitofp i16 and then fptosi i32.
Done, thanks for guiding me on how to build the right test cases.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129958/new/

https://reviews.llvm.org/D129958