[PATCH] D36592: [BDCE] clear poison generators after turning a value into zero (PR33695, PR34037)

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Transforms/Scalar/BDCE.cpp:46
+  // demanded.
+  // TODO: "Any value in the def/use chain that has all bits set as non-dead
+  //        (by the demanded bit analysis) cannot have its value changed: if all
----------------
I think this is better than the opcode list I suggested. Why don't we just do this instead? Isn't it just something like this?

  auto isExternallyVisible = [&](Instruction *I) {
    // Any value we're trivializing should be an integer value, and moreover, any conversion between an integer value and a non-integer value should demand all of the bits, which will cause us to stop looking down the use/def chain, so we should only see integer-typed instructions here.
    assert(I->getType()->isIntegerTy() && "Trivializing a non-integer value?");
    return DB.getDemandedBits(I).isAllOnesValue();
  }



================
Comment at: lib/Transforms/Scalar/BDCE.cpp:80
+    J->dropPoisonGeneratingFlags();
+    // FIXME: llvm.assume (and range metadata?) may also be invalid now.
+
----------------
Range metadata can't be invalid here because it applies only to memory accesses (which demand all bits). As a result, I'd like to not see it in a TODO, although a comment explaining why it's not an issue would be good.


================
Comment at: lib/Transforms/Scalar/BDCE.cpp:80
+    J->dropPoisonGeneratingFlags();
+    // FIXME: llvm.assume (and range metadata?) may also be invalid now.
+
----------------
hfinkel wrote:
> Range metadata can't be invalid here because it applies only to memory accesses (which demand all bits). As a result, I'd like to not see it in a TODO, although a comment explaining why it's not an issue would be good.
Please just include the assume check if it is not complicated. I think this will work:

  if (auto *II = dyn_cast<IntrinsicInst>(J)) {
    if (II->getIntrinsicID() == Intrinsic::assume)
     // We can delete the assume here because it can't appear in the worklist multiple times (because it only has one operand).
     II->eraseFromParent();
   }



https://reviews.llvm.org/D36592