[PATCH] D40554: [PowerPC] Fix bugs in sign-/zero-extension elimination

[Nemanja Ivanovic via Phabricator via llvm-commits]

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

To be honest, this code seems even more counter-intuitive to me now. I initially thought that this should essentially be a known-bits calculation on virtual registers and suggested that. With these changes, the code even more closely resembles that, but in a  very roundabout fashion. Seems like it would be much simpler to implement and follow this if we just built and maintained a map from virtual registers to known zero/sign bits. Then all we need to check in order to decide if we want to get rid of an `EXTS[BHW]` is whether the correct number of leading bits are known to be sign bits. Similarly, we'd know if it is safe to remove a `CLRLDI/RLWINM/...` if the correct number of leading bits are known to be zero. Furthermore, with this approach we wouldn't need to consider the depth that we look through.

Of course, we can leave that as a future simplification.

The reason I've requested changes is to get rid of the debugging code and change the names of the functions.



================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:2164
 // i.e. 0 to 31-th bits are same as 32-th bit.
-static bool isSignExtendingOp(const MachineInstr &MI) {
-  int Opcode = MI.getOpcode();
+static bool isSignExtendingOp(const unsigned Reg,
+                              const MachineRegisterInfo *MRI) {
----------------
Similarly to `isZeroExtendingOp()`, this name no longer makes sense.


================
Comment at: lib/Target/PowerPC/PPCInstrInfo.cpp:2215
 // always outputs zeros in higher 32 bits.
-static bool isZeroExtendingOp(const MachineInstr &MI) {
-  int Opcode = MI.getOpcode();
+static bool isZeroExtendingOp(const unsigned Reg,
+                              const MachineRegisterInfo *MRI) {
----------------
This name no longer makes sense. This is really a query for whether or not the virtual register contains a value that is zero-extended.


================
Comment at: lib/Target/PowerPC/PPCMIPeephole.cpp:196
 
+      if (!MI.isPHI())
+        for (auto & Use : MI.uses()) {
----------------
This whole section needs to be cleaned up. Lines too long. Code commented-out. Pre-processor conditions. Presumably leftovers from debugging during development.

Actually, on second look, this injects trap instructions for debugging and should probably just be removed.


https://reviews.llvm.org/D40554