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

[Esme Yi via Phabricator via llvm-commits]

Esme added a comment.

In D131374#3767476 <https://reviews.llvm.org/D131374#3767476>, @nemanjai wrote:

> 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.

I had the concern too, and tried to put this conversion elsewhere, like in ISelLowering (as D98542 <https://reviews.llvm.org/D98542>), but found some case could therefore miss the optimization in `PPCMIPeephole::eliminateRedundantCompare( )`. 
Performing a canonicalization in a function for optimization does change the instruction but doesn't perform any optimization behavior, so I think it doesn't changes the meaning of the return value (i.e. whether the optimization was performed successfully). 
Although the patch seems not the most perfect approach, but this is a rather appropriate approach after a trade-off.

> 1. 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.

Yes, this canonicalization will not affect following paths in this function.

> 1. 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.

Both SPEC and bootstrap are clean after applying this patch.

> 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.

Thanks for your approve. The return value of `true` for this function means the optimization of comparison is performed successfully (ie. the comparison is eliminated), therefore the canonicalization should not change the return value.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131374