[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;
----------------
XChy wrote:
> goldstein.w.n wrote:
> > 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.
> Thanks for your comment! I agree with you. It's not a good way to erase some deferred instructions in a fold function. It's better to let `InstCombinerImpl::run()` control the logic of instruction erasion.
> 
> But there aren't other ways to determine whether some instructions can be folded  by `fold<BinOp>OfICmp`, unless we just call it with the instructions **built and deferred** or we can extract a **canFold<BinOp>OfICmp** function to take the components.
> 
> The latter costs too much, since it may require refactoring all sub folds.
> 
> For that reason, I applied the former but I didn't find any similar situations in **InstCombine**. I just try to copy some of the logic of instructions erasion to solve the problem.
> 
> I'll search for more similar cases to get a better solution if possible. I'll highly appreciate it if you could give me some other advice.
Unfortunately I don't really have a better idea than what you have here, but want to here nikic's opinion


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

https://reviews.llvm.org/D154791