Page MenuHomePhabricator

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

Authored by Esme on Aug 7 2022, 9:38 PM.

Details

Summary

Converting a comparison against 1 or -1 into a comparison against 0 can exploit record-form instructions for comparison optimization. The conversion will happen only when a record-form instruction can be used to replace the comparison during the peephole optimizer (see function optimizeCompareInstr).

In post-RA, we also want to optimize the comparison by using the record form (see D131873) and it requires additional dataflow analysis to reliably find uses of the CR register set. It's reasonable to common the conversion for both peephole optimizer and post-RA optimizer.

Converting to comparison against zero even when the optimization doesn't happened in peephole optimizer may create additional opportunities for the post-RA optimization.

Diff Detail

Event Timeline

Esme created this revision.Aug 7 2022, 9:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2022, 9:38 PM
Esme requested review of this revision.Aug 7 2022, 9:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2022, 9:38 PM
Esme retitled this revision from [PowerPC] Modify the condition code in peephole optimi to [PowerPC] Modify the condition code in peephole optimization..Aug 7 2022, 9:48 PM
Esme edited the summary of this revision. (Show Details)
Esme added reviewers: Restricted Project, shchenz, nemanjai.
nemanjai requested changes to this revision.Aug 8 2022, 4:54 AM

I don't really have an issue with the code in this patch. Converting to comparison against zero whenever possible (even when a record-form instruction cannot be used to replace the comparison) seems perfectly reasonable.
However, I don't want this to go in without a good explanation - and the explanation you provided is inadequate.

  • There is no mention of why this conversion doesn't already happen (i.e. no opportunity exists to produce a record-form instruction)
  • There isn't an adequate description of what you hope to do with these in post-RA optimizations
  • The post-RA patch should be posted first and linked with this one so that it is clear what the relationship is between the optimizations
  • The description makes it seem like the future post-RA optimization depends on this which is not OK - it is OK for this to create additional opportunities for the post-RA optimization
llvm/lib/Target/PowerPC/PPCInstrInfo.cpp
2541–2546

"Modify the condition code..." is too vague. You want to state what is actually being changed here:

// Convert the comparison and its user to a compare against zero with
// the appropriate predicate on the branch. Zero comparison might provide
// optimization opportunities post-RA (see optimization in 
// PPCPreEmitPeephole.cpp).

(assuming that is where the post-RA transformation you're referring to is).

This revision now requires changes to proceed.Aug 8 2022, 4:54 AM
Esme updated this revision to Diff 452593.Aug 15 2022, 12:25 AM
Esme edited the summary of this revision. (Show Details)
Esme added a comment.Aug 15 2022, 12:28 AM

Addressed @nemanjai 's comments. Thx!

  1. updated the description.
  2. updated the comment.
  3. posted patch D131873 for comparison optimization in post-RA.
Esme retitled this revision from [PowerPC] Modify the condition code in peephole optimization. to [PowerPC] Converting to comparison against zero even when the optimization doesn't happened in peephole optimizer..Aug 15 2022, 12:38 AM
nemanjai accepted this revision.Sep 2 2022, 11:27 AM

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.

This revision is now accepted and ready to land.Sep 2 2022, 11:27 AM
Esme added a comment.Tue, Sep 13, 3:38 AM

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