[PATCH] D57085: [RISCV] Custom-legalise 32-bit variable shifts on RV64

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added inline comments.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:541
+  SDValue NewRes = DAG.getNode(WOpcode, DL, MVT::i64, NewOp0, NewOp1);
+  // We need to replace the i32 node with another i32 node. Replacing with
+  // an i64 node doesn't trigger an assert, but does lead to problems in the
----------------
The documentation of `ReplaceNodeResults` already states this requirement of matching types between the original node and the new node. Looks to me the elaborated explanation here regarding the ty->ty no-op extends might be confusing.

I think we can go with a simple reminder that explains the final truncate.


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:680
 def shiftwamt_mask : ImmLeaf<XLenVT, [{
   return countTrailingOnes<uint64_t>(Imm) >= 5 && isUInt<32>(Imm);
 }]>;
----------------
I don't understand the check `isUInt<32>(Imm)` and I suspect that given where this fragment is used (only in `riscv_s*w` nodes) this may not be needed.

In fact, I removed the check and the existing tests still pass. If this is actually needed we may be missing some coverage here.


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

https://reviews.llvm.org/D57085