[PATCH] D119532: [RISCV] Extend sext.w removal pass to remove unused sign-extensions

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp:63
+  for (auto &UserOp : MRI.reg_operands(MI.getOperand(0).getReg())) {
+    const auto User = UserOp.getParent();
+    if (User == &MI) // ignore the def, current MI
----------------
`const auto` -> `const auto *`. LLVM coding guidelines want to see the *


================
Comment at: llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp:149
+
+    case RISCV::ADD_UW:
+    case RISCV::ANDN:
----------------
Only one of the operands of ADD_UW uses all bits. Can we know which operand is being used?


================
Comment at: llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp:162
+    case RISCV::SH3ADD_UW:
+    case RISCV::SLLI_UW:
+    case RISCV::XNOR:
----------------
SLLI_UW could be treated the same as W instructions. It only uses the lower 32 bits of the first operand.


================
Comment at: llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp:180
 // TODO: Add other W instructions.
 static bool isSignExtendingOpW(const MachineInstr &MI) {
   switch (MI.getOpcode()) {
----------------
Can you put the changes to this function in a separate patch with tests?


================
Comment at: llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp:396
+      // then sext.w is unnecessary
+      if (!isAllUsesReadW(*MI, MRI)) {
+        // If all definitions reaching MI sign-extend their output,
----------------
Combine this into a single `if` using &&


================
Comment at: llvm/test/CodeGen/RISCV/sextw-removal.ll:506
+bb2:                                              ; preds = %bb2, %bb
+  %i1 = phi i64 [ %arg1, %entry ], [ %i5, %bb2 ]
+  %i2 = shl i64 %i1, 32
----------------
craig.topper wrote:
> The loop calculate the same value every time. The loop optimizers in the middle can remove it.
> 
> Nothing about this test looks a problem that is unique to RISCV. The shifts are redundant on any target and are visible to the middle end. Why shouldn't we clean it up in the middle end?
I still don't think this is a robust test. The middle end can make it not a loop so it would never make it to the backend in this form. And it doesn't demonstrate a RISCV specific problem This code is a problem for any target so why is a RISC-V specific solution that correct solution?


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

https://reviews.llvm.org/D119532