[PATCH] D129191: [IndVars] Eliminate redundant type cast between integer and float

[Allen zhong via Phabricator via llvm-commits]

Allen marked 5 inline comments as done.
Allen added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyIndVar.cpp:679
+bool SimplifyIndvar::replaceFloatIVWithIntegerIV(Instruction *UseInst,
+                                                 Value *IVOperand) {
+  if (UseInst->getOpcode() != CastInst::SIToFP)
----------------
nikic wrote:
> We don't really need the IVOperand argument, given how casts only have a single operand.
Done


================
Comment at: llvm/lib/Transforms/Utils/SimplifyIndVar.cpp:694
+
+      CI->replaceAllUsesWith(IVOperand);
+      LLVM_DEBUG(dbgs() << "INDVARS: Replace IV user: " << *CI
----------------
nikic wrote:
> We should also add `CI` to `DeadInsts`, so it gets DCEd.
Thanks @nikic very much, apply your comment.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyIndVar.cpp:699
+      ++NumFoldedUser;
+      Changed |= true;
+    }
----------------
nikic wrote:
> Can just be `Changed = true`, no need for `|=`.
Done


================
Comment at: llvm/test/Transforms/IndVarSimplify/floating-point-small-iv.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -indvars -adce -S | FileCheck %s
+
----------------
nikic wrote:
> Drop the `-adce` here.
Done, it can be dropped after add CI to **DeadInsts** according your comment, thanks.


================
Comment at: llvm/test/Transforms/IndVarSimplify/floating-point-small-iv.ll:36
+  ret void
+}
+
----------------
nikic wrote:
> For this test, it would be better to use an integer IV from the start, rather than going through the float -> int transform. This makes it clear that your code works even if IndVars is not the producer of the integer IV (which is the key difference to D129140).
Done


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

https://reviews.llvm.org/D129191