[PATCH] D11596: Separate out BDCE's analysis into a separate DemandedBits analysis.

[hfinkel@anl.gov via llvm-commits]

hfinkel added inline comments.

================
Comment at: include/llvm/Analysis/DemandedBits.h:54
@@ +53,3 @@
+  /// Return true if, during analysis, I could not be reached.
+  bool wasInstructionDead(Instruction *I);
+
----------------
I think that I prefer the present tense for analysis results, let's name this:

  isInstructionDead

================
Comment at: lib/Analysis/DemandedBits.cpp:351
@@ +350,3 @@
+APInt DemandedBits::getDemandedBits(Instruction *I) {
+  assert(I->getType()->isIntegerTy() && "Only integer types supported!");
+  if (AliveBits.count(I))
----------------
Given that you return a conservative answer regardless, I don't see any reason to have this assert. I recommend removing it (and the associated comment in the header).

================
Comment at: lib/Analysis/DemandedBits.cpp:354
@@ +353,3 @@
+    return AliveBits[I];
+  return APInt::getAllOnesValue(I->getType()->getScalarSizeInBits());
+}
----------------
Use getSizeInBits instead of getScalarSizeInBits so that we get the right conservative answer for vectors.

================
Comment at: lib/Transforms/Scalar/BDCE.cpp:82
@@ -376,1 +81,3 @@
+      if (DB.demandedBitsKnown(&I)) {
+        if (DB.getDemandedBits(&I).getBoolValue())
           continue;
----------------
Is looks like, as you have it setup, getDemandedBits will return all ones for an instruction not in the map (for any instruction for which DB.demandedBitsKnown(&I) would return false). As a result, we don't need the DB.demandedBitsKnown(&I) check and can just use the DB.getDemandedBits check (avoiding repeating the map lookup).



Repository:
  rL LLVM

http://reviews.llvm.org/D11596