[PATCH] D131374: [PowerPC] Converting to comparison against zero even when the optimization doesn't happened in peephole optimizer.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

On the surface, this patch is fine. I don't think it should cause any issues. However, there are some important implications that you need to consider here.

1. There are paths out of this function where we can return `false` even though we have actually modified code. The convention for functions that perform and optimization and return a Boolean to state whether they successfully performed the optimization is to not modify code at all if they are returning `false` (indicating the optimization was not successful). You are fundamentally changing that for this function. Is that OK? Well, I don't know. It depends on what assumptions the caller (and recursively the conceptual call stack) makes wrt. to the return value - both now and in the future. Ultimately, optimization and canonicalization are distinct. This is a function that is expected to perform an optimization. But it performs a canonicalization in a superset of cases for which it performs an optimization. And the answer to the question "What should it return if it performs a canonicalization and not an optimization?" is not easy to formulate.
2. There is a lot of code with diverging paths in this function after the canonicalization you perform here. Have you verified that none of it depends on the def and use have not been modified? As a reviewer, I certainly don't have the time to track through the entire large function and confirm this. But it needs to be done.
3. Although this seems like a rather benign change, this is potentially rather pervasive. As such, this patch needs to be tested extremely thoroughly. Certainly execution tests using SPEC, LLVM bootstrap and perhaps some other software packages that we can use for testing.

As I am not opposed to this patch, I will approve it since further reviews will not reveal any new useful information. However, I trust that you will consider these comments and perform the testing and analysis of the surrounding code prior to committing this. If it turns out that the early returns after the canonicalization need to change from `false` to `true` and you would prefer another review, feel free to request another review. Also, please provide details as a comment here as to what testing you performed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131374