[PATCH] "float2int": Add a new pass to demote from float to int where possible.

[hfinkel at anl.gov]

This is neat, thanks for posting it!


REPOSITORY
  rL LLVM

================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:46
@@ +45,3 @@
+/// The largest integer type worth dealing with.
+static const unsigned MaxIntegerBW = 64;
+
----------------
Make this a command-line option.

================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:156
@@ +155,3 @@
+  switch (I->getOpcode()) {
+  default:
+    // Path terminated uncleanly.
----------------
You can also handle Select here (and PHIs, although that might require more work?)

================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:223
@@ +222,3 @@
+      ECs.unionSets(I, OI);
+      OpRanges.push_back(traverse(OI));
+      // Early exit if we just received poison - no use continuing.
----------------
This is directly recursive; that's likely a bad idea (especially considering that it has no depth cutoff). Can you make this be worklist-based instead. We don't need a small cutoff if we have a worklist.

================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:255
@@ +254,3 @@
+      // FIXME: false->true?
+      APSInt Int(MaxIntegerBW+1, false);
+      bool Exact;
----------------
Why is the FIXME here? Do you want to mark these as unsigned under some circumstances?

================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:280
@@ +279,3 @@
+  for (auto It = ECs.begin(), E = ECs.end(); It != E; ++It) {
+    // FIXME: false->true?
+    ConstantRange R(MaxIntegerBW + 1, false);
----------------
What's this FIXME for? You'd want to mark the full range? (that would make the unioning unnecessary ;) )

================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:317
@@ +316,3 @@
+    if (ECs.member_begin(It) == ECs.member_end() || Fail ||
+        R.isFullSet() || R.isSignWrappedSet())
+      continue;
----------------
Why are you checking for isFullSet() here? What about if you only have i8 (or something definitively smaller than the mantissa)?

================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:332
@@ +331,3 @@
+    // to? Floats have 23 bits of mantissa, Doubles have 52 bits.
+    unsigned MaxRepresentableBits = ConvertedToTy->isFloatTy() ? 23 : 52;
+    if (MinBW > MaxRepresentableBits) {
----------------
Hrmm, we have other floating-point types in the IR. You need to either exclude them somewhere, or handle them.

Either way, the real way to do this is to call:
  ... = APFloat::semanticsPrecision(ConvertedToTy->getFltSemantics()) - 1;


================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:340
@@ +339,3 @@
+    // FIXME: Pick the smallest legal type that will fit.
+    Type *Ty = (MinBW > 23) ? Type::getInt64Ty(*Ctx) : Type::getInt32Ty(*Ctx);
+
----------------
Exactly where does 23 come from here? I understand that is the number of bits in a single-precision mantissa, but what does it have to do with picking the integer type?

Can you use make use of Type::getIntNTy?


================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:384
@@ +383,3 @@
+  case Instruction::FPToUI:
+    if (I->getType() == NewOperands[0]->getType())
+      NewV = NewOperands[0];
----------------
You should not need to explicitly handle the equal-types case here. IRB::CreateZExtOrTrunc, will also check this and do nothing if the incoming and outgoing bitwidths are equal.

(this obviously applies to the checks below as well).


================
Comment at: lib/Transforms/Scalar/Float2Int.cpp:441
@@ +440,3 @@
+       I != E; ++I) {
+    if (I->first->use_empty())
+      I->first->eraseFromParent();
----------------
Hrmm. To have converted an instruction, you must have converted all uses, right? If we know these need to be dead, and we just don't know the order, we can use the same technique employed by ADCE:

  1. First, loop over all instructions calling I->dropAllReferences
  2. Then, loop over them again, erasing them as you do here.

================
Comment at: test/Transforms/Float2Int/basic.ll:3
@@ +2,3 @@
+
+;Test plan:
+;  positive tests
----------------
Please move these descriptions to be with each associated test.

http://reviews.llvm.org/D7790

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/