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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

This looks reasonable to me, but I think we could use at least two more tests: 1. Type mismatch between the integer IV and the fptosi result. 2. Case where the transform is *not* valid because there are too many significant bits.



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


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


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


================
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
+
----------------
Drop the `-adce` here.


================
Comment at: llvm/test/Transforms/IndVarSimplify/floating-point-small-iv.ll:36
+  ret void
+}
+
----------------
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).


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

https://reviews.llvm.org/D129191