[PATCH] D154791: [InstCombine] Transform bitwise (A >> C - 1, zext(icmp)) -> zext (bitwise(A < 0, icmp)) fold.

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1754
+    Worklist.popDeferred()->eraseFromParent();
+    Worklist.popDeferred()->eraseFromParent();
+    return nullptr;
----------------
I'm mostly okay with the change, but a little unhappy about this. It seems like a worrisome practice.
I guess it works, but it would be nice if there where a better way to accomplish it. 

Generally I'd argue the best solution would be to refactor `fold<BinOp>OfICmp` to take the components rather than the final instructions, but those are both fairly complicated and propagate the instructions to other functions that would then need to be refactored. All in all more work than its worth.

I'm going to defer my opinion to @nikic about whether this is okay.

Other than my concern here, I'm basically ready to approve.


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

https://reviews.llvm.org/D154791