[PATCH] D131374: [PowerPC] Modify the condition code in peephole optimization.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai requested changes to this revision.
nemanjai added a comment.
This revision now requires changes to proceed.

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



================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.cpp:2540
 
-    PredsToUpdate.push_back(std::make_pair(&(UseMI->getOperand(0)), Pred));
+    // Modify the condition code directly even though the CmpInstr may not be
+    // optimized. There is no harm in doing so, and may create opportunities for
----------------
"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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131374