[PATCH] D90961: [RISCV] When matching SROIW, check all 64 bits of the OR mask

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck accepted this revision.
frasercrmck added a comment.
This revision is now accepted and ready to land.

Apart from my nits, this makes sense to me. The only thing I don't fully get it where/why the original code asserted if the shift amount was >= 32.

Regarding `SLOIW` I'm not sure you'll find a test case where the mask isn't wholly contained in the 32 lower bits since the or and the shl are `i32` operations. But I don't think it hurts to check all 64 bits.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:317
 //
-//  VC1 == maskTrailingOnes<uint32_t>(VC2)
+//  VC1 == maskTrailingOnes<uint64_t>(VC2)
 
----------------
Your changes to SROIW also mention the constraint `VC2 < 32` so I reckon it should be stated here too


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:320
 bool RISCVDAGToDAGISel::SelectSLOIW(SDValue N, SDValue &RS1, SDValue &Shamt) {
-  if (Subtarget->getXLenVT() == MVT::i64 &&
-      N.getOpcode() == ISD::SIGN_EXTEND_INREG &&
-      cast<VTSDNode>(N.getOperand(1))->getVT() == MVT::i32) {
-    if (N.getOperand(0).getOpcode() == ISD::OR) {
-      SDValue Or = N.getOperand(0);
-      if (Or.getOperand(0).getOpcode() == ISD::SHL) {
-        SDValue Shl = Or.getOperand(0);
-        if (isa<ConstantSDNode>(Shl.getOperand(1)) &&
-            isa<ConstantSDNode>(Or.getOperand(1))) {
-          uint32_t VC1 = Or.getConstantOperandVal(1);
-          uint32_t VC2 = Shl.getConstantOperandVal(1);
-          if (VC1 == maskTrailingOnes<uint32_t>(VC2)) {
-            RS1 = Shl.getOperand(0);
-            Shamt = CurDAG->getTargetConstant(VC2, SDLoc(N),
-                                              Shl.getOperand(1).getValueType());
-            return true;
-          }
-        }
-      }
-    }
-  }
-  return false;
+  assert(Subtarget->is64Bit() && "SROIW should only be matched on RV64");
+  if (N.getOpcode() != ISD::SIGN_EXTEND_INREG ||
----------------
Comment here should say "SLOIW"


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

https://reviews.llvm.org/D90961