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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.
Herald added subscribers: alextsao1999, eopXD.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:797
     return UnableToLegalize;
+  case TargetOpcode::G_MUL:
   case TargetOpcode::G_SDIV:
----------------
Can you split the mul libcall handling into a separate patch?


================
Comment at: llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp:146
+  Register NewOp0 = MRI.createGenericVirtualRegister(s64);
+  MIRBuilder.buildAnyExt({NewOp0}, {MI.getOperand(1).getReg()});
+  Register NewOp1 = MRI.createGenericVirtualRegister(s64);
----------------
You can do auto NewOp0 = MIRBuilder.buildAnyExt(s64, MI.getOperand(1).getReg())


================
Comment at: llvm/lib/Target/RISCV/RISCVLegalizerInfo.cpp:181
+
+bool RISCVLegalizerInfo::legalizeWOp(MachineInstr &MI, MachineRegisterInfo &MRI,
+                                     MachineIRBuilder &MIRBuilder) const {
----------------
You're not really legalizing anything here, you're just directly selecting which I would advise against


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/legalizer/alu32.mir:9
+  ; Extends only exhaustively tested for add to avoid excessive tests.
+  define void @add_i8() { entry: ret void }
+  define void @add_i8_signext() { entry: ret void }
----------------
lewis-revill wrote:
> arsenm wrote:
> > You don't need the IR section here
> I didn't realise this, though I'm considering leaving it for consistency with tests in other backends? Also I am going to look into using the update_mir_test_checks tool instead.
No, just delete it. Nearly every IR section can be deleted


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76354