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

[Hongyu Chen via Phabricator via llvm-commits]

XChy added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1754
+    Worklist.popDeferred()->eraseFromParent();
+    Worklist.popDeferred()->eraseFromParent();
+    return nullptr;
----------------
nikic wrote:
> goldstein.w.n wrote:
> > 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
> We shouldn't do this kind of speculative transform. Either you can always do the transform based on a reasonably close heuristic (like zext and icmp on the same value) or change APIs in a way that allows doing the transform without materializing the icmp (I think this is not worth the trouble).
> 
> My 2c here is that it would be okay to convert to the zext(bitwise(icmp, icmp)) form even if it doesn't always fold, as this seems like the better representation at the IR level to me. Even if it doesn't fold in InstCombine, this is the form that is more likely to be usefully optimized by other passes. If we really care, we can undo this in the backend.
I see. I have reverted to the version producing `zext(bitwise(icmp.icmp))`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154791