[PATCH] D58877: [InstCombine] fold add(add(A, ~B), 1) -> sub(A, B)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In D58877#1426695 <https://reviews.llvm.org/D58877#1426695>, @deadalnix wrote:

> @spatel I'm not sure, you tell me :) The history of this patch is that this pattern shows up in practice post legalization, so I submitted a patch for it for DAGCombiner. From there, @ lebedev.ri noticed that InstCombine wasn't doing it and suggested it to be added there as well.
>
> As far as I can tell, -reassociate also fails to trigger this transform: https://godbolt.org/z/gweK6j


You have to run reassociate before instcombine to get the expected ranking of operands:
https://godbolt.org/z/5XHhTh

So I agree that we needed the backend patch, but I don't see any evidence that this is needed within instcombine yet. The difference is that - in the backend - DAGCombiner is doing the job of both of the IR passes (reassociate is a canonicalization pass that works with instcombine). If we don't see this pattern after a normal IR optimization sequence, then I don't think we want to add it to instcombine (it would likely infinitesimally increase compile-time with no potential benefit). One way to see if there's any benefit to this patch would be to add a transform statistic, build the test-suite or some other substantial codebase, and see if this transform ever fires.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58877