[RISCV] Partially move doPeepholeMaskedRVV into RISCVFoldMasks

[preames]

This change is motived by a point of confusion on #71764. I hadn't fully understood why doPeepholeMaskedRVV needed to be part of the same change. As indicated in the fixme in this patch, the reason is that performCombineVMergeAndVOps doesn't know how to deal with the true side of the merge being a all-ones masked instruction.

This change removes one of two calls to the routine in RISCVISELDAGToDAG, and adds a clarifying comment on the precondition for the remaining call. The post-ISEL code is tested by the cases where we can form a unmasked instruction after folding the vmerge back into true.

I don't really care if we actually land this patch, or leave it roled into #71764. I'm posting it mostly to clarify the confusion.

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

Author: Philip Reames (preames)

Changes

This change is motived by a point of confusion on #71764. I hadn't fully understood why doPeepholeMaskedRVV needed to be part of the same change. As indicated in the fixme in this patch, the reason is that performCombineVMergeAndVOps doesn't know how to deal with the true side of the merge being a all-ones masked instruction.

This change removes one of two calls to the routine in RISCVISELDAGToDAG, and adds a clarifying comment on the precondition for the remaining call. The post-ISEL code is tested by the cases where we can form a unmasked instruction after folding the vmerge back into true.

I don't really care if we actually land this patch, or leave it roled into #71764. I'm posting it mostly to clarify the confusion.

---

Full diff: https://github.com/llvm/llvm-project/pull/72441.diff

2 Files Affected:

• (modified) llvm/lib/Target/RISCV/RISCVFoldMasks.cpp (+46)
• (modified) llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp (+4-2)

diff --git a/llvm/lib/Target/RISCV/RISCVFoldMasks.cpp b/llvm/lib/Target/RISCV/RISCVFoldMasks.cpp
index 63849238f9ec7c2..4586a4a0c72ed39 100644
--- a/llvm/lib/Target/RISCV/RISCVFoldMasks.cpp
+++ b/llvm/lib/Target/RISCV/RISCVFoldMasks.cpp
@@ -18,6 +18,7 @@
 
 #include "RISCV.h"
 #include "RISCVSubtarget.h"
+#include "RISCVISelDAGToDAG.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
@@ -48,6 +49,7 @@ class RISCVFoldMasks : public MachineFunctionPass {
   StringRef getPassName() const override { return "RISC-V Fold Masks"; }
 
 private:
+  bool convertToUnmasked(MachineInstr &MI, MachineInstr *MaskDef);
   bool convertVMergeToVMv(MachineInstr &MI, MachineInstr *MaskDef);
 
   bool isAllOnesMask(MachineInstr *MaskDef);
@@ -132,6 +134,49 @@ bool RISCVFoldMasks::convertVMergeToVMv(MachineInstr &MI, MachineInstr *V0Def) {
   return true;
 }
 
+bool RISCVFoldMasks::convertToUnmasked(MachineInstr &MI,
+                                       MachineInstr *MaskDef) {
+  const RISCV::RISCVMaskedPseudoInfo *I =
+      RISCV::getMaskedPseudoInfo(MI.getOpcode());
+  if (!I)
+    return false;
+
+  if (!isAllOnesMask(MaskDef))
+    return false;
+
+  // There are two classes of pseudos in the table - compares and
+  // everything else.  See the comment on RISCVMaskedPseudo for details.
+  const unsigned Opc = I->UnmaskedPseudo;
+  const MCInstrDesc &MCID = TII->get(Opc);
+  const bool HasPolicyOp = RISCVII::hasVecPolicyOp(MCID.TSFlags);
+  const bool HasPassthru = RISCVII::isFirstDefTiedToFirstUse(MCID);
+#ifndef NDEBUG
+  const MCInstrDesc &MaskedMCID = TII->get(MI.getOpcode());
+  assert(RISCVII::hasVecPolicyOp(MaskedMCID.TSFlags) ==
+             RISCVII::hasVecPolicyOp(MCID.TSFlags) &&
+         "Masked and unmasked pseudos are inconsistent");
+  assert(HasPolicyOp == HasPassthru && "Unexpected pseudo structure");
+#endif
+
+  MI.setDesc(MCID);
+
+  // TODO: Increment all MaskOpIdxs in tablegen by num of explicit defs?
+  unsigned MaskOpIdx = I->MaskOpIdx + MI.getNumExplicitDefs();
+  MI.removeOperand(MaskOpIdx);
+
+  // The unmasked pseudo will no longer be constrained to the vrnov0 reg class,
+  // so try and relax it to vr.
+  MRI->recomputeRegClass(MI.getOperand(0).getReg());
+  unsigned PassthruOpIdx = MI.getNumExplicitDefs();
+  if (HasPassthru) {
+    if (MI.getOperand(PassthruOpIdx).getReg() != RISCV::NoRegister)
+      MRI->recomputeRegClass(MI.getOperand(PassthruOpIdx).getReg());
+  } else
+    MI.removeOperand(PassthruOpIdx);
+
+  return true;
+}
+
 bool RISCVFoldMasks::runOnMachineFunction(MachineFunction &MF) {
   if (skipFunction(MF.getFunction()))
     return false;
@@ -159,6 +204,7 @@ bool RISCVFoldMasks::runOnMachineFunction(MachineFunction &MF) {
     CurrentV0Def = nullptr;
     for (MachineInstr &MI : MBB) {
       unsigned BaseOpc = RISCV::getRVVMCOpcode(MI.getOpcode());
+      Changed |= convertToUnmasked(MI, CurrentV0Def);
       if (BaseOpc == RISCV::VMERGE_VVM)
         Changed |= convertVMergeToVMv(MI, CurrentV0Def);
 
diff --git a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
index e1375f05cdecdc7..0429e0692252156 100644
--- a/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
@@ -151,6 +151,10 @@ void RISCVDAGToDAGISel::PostprocessISelDAG() {
       continue;
 
     MadeChange |= doPeepholeSExtW(N);
+
+    // FIXME: This is here only because the VMerge transform doesn't
+    // know how to handle masked true inputs.  Once that has been moved
+    // to post-ISEL, this can be deleted as well.
     MadeChange |= doPeepholeMaskedRVV(cast<MachineSDNode>(N));
   }
 
@@ -3711,8 +3715,6 @@ bool RISCVDAGToDAGISel::performCombineVMergeAndVOps(SDNode *N) {
   for (unsigned Idx = 1; Idx < True->getNumValues(); ++Idx)
     ReplaceUses(True.getValue(Idx), SDValue(Result, Idx));
 
-  // Try to transform Result to unmasked intrinsic.
-  doPeepholeMaskedRVV(Result);
   return true;
 }
 

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff c70f6fc4f4c33656b2048e37c287d5c811f413b2 d4cd325ae9351fd288eea248a54eb5a06fe58e9c -- llvm/lib/Target/RISCV/RISCVFoldMasks.cpp llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/Target/RISCV/RISCVFoldMasks.cpp b/llvm/lib/Target/RISCV/RISCVFoldMasks.cpp
index 4586a4a0c7..dad11cb6ec 100644
--- a/llvm/lib/Target/RISCV/RISCVFoldMasks.cpp
+++ b/llvm/lib/Target/RISCV/RISCVFoldMasks.cpp
@@ -17,8 +17,8 @@
 //===---------------------------------------------------------------------===//
 
 #include "RISCV.h"
-#include "RISCVSubtarget.h"
 #include "RISCVISelDAGToDAG.h"
+#include "RISCVSubtarget.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"

[lukel97]

LGTM, no strong opinion on which one to merge

[topperc]

Alphabetize? Though I'm surprised a SelectionDAG header is needed in an MIR pass.

[lukel97]

It's needed for the masked pseudos table, which after everything else is moved over can also be moved into RISCVFoldMasks

[lukel97]

See also https://github.com/lukel97/llvm-project/blob/01854599f2cfd84899c37f4791d6c4a0784017e1/llvm/lib/Target/RISCV/RISCVFoldMasks.cpp#L521-L522