[PATCH] D76051: [WIP][RISCV][GlobalISel] Select register banks for GPR ALU instructions

[Matt Arsenault via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterBankInfo.cpp:95
+
+  bool IsRV64 = MF.getSubtarget<RISCVSubtarget>().is64Bit();
+
----------------
I'd recommend keeping the subtarget cached in the class


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterBankInfo.cpp:99
+  // TODO: Fixed size FPR registers on both RV32/RV64.
+  for (const auto &Op : MI.operands()) {
+    if (Op.isReg()) {
----------------
I don't see the point of this loop. It seems like you're just doing a rough legality check, but the function is already required (and enforced by the verifier) to be legal at this point


================
Comment at: llvm/lib/Target/RISCV/RISCVRegisterBankInfo.cpp:102
+      LLT RegTy = MRI.getType(Op.getReg());
+      // Checking isScalar also filters out $noreg.
+      if (!RegTy.isScalar())
----------------
You shouldn't see noreg on a generic instruction so I'm not sure what you're addressing here


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu32.mir:7
+
+  define void @add_anyext() { entry: ret void }
+  define void @add_signext() { entry: ret void }
----------------
You can drop the IR section


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76051