[PATCH] D76354: [RISCV][GlobalISel] Legalize types for ALU operations

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp:101-104
+bool RISCVLegalizerInfo::legalizeWOpWithSExt(
+    MachineInstr &MI, MachineRegisterInfo &MRI,
+    MachineIRBuilder &MIRBuilder) const {
+  const LLT s64 = LLT::scalar(64);
----------------
You shouldn't have this function, you're just repeating totally ordinary promotion the legalizer will handle by default


================
Comment at: llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp:120
+
+bool RISCVLegalizerInfo::legalizeWOp(MachineInstr &MI, MachineRegisterInfo &MRI,
+                                     MachineIRBuilder &MIRBuilder) const {
----------------
Ditto, you're repeating basic work the legalizer does by default


================
Comment at: llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp:105
+  // TODO: We have better patterns for this depending on the operand.
+  getActionDefinitionsBuilder(G_SEXT_INREG).lower();
+
----------------
lewis-revill wrote:
> Currently RISC-V defines signext_inreg as legal when we know that the inner type is i32. However with GlobalISel there is no way to retrieve the inner type to define legality, as the action of G_SEXT_INREG is determined by an immediate operand.
> 
> I was currently in the process of rewriting these patches to properly handle single word operations on RV64, of which ADDW, SUBW and MULW rely on pattern matching an i32 to i64 signext_inreg of the corresponding operation. Though since we cannot allow the G_SEXT_INREG to survive correctly I feel the only option for now may be to directly select these instructions at the legalization stage.
The immediate and the inner type are equivalent representations of the same thing. You can and should define the legality rules the same.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76354