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

[Nitin John Raj via Phabricator via llvm-commits]

nitinjohnraj added inline comments.


================
Comment at: llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp:123
+    LLT Ty = MRI.getType(MI.getOperand(0).getReg());
+    OperandsMapping =
+        getOperandsMapping({getGPRValueMapping(Ty.getSizeInBits()), nullptr});
----------------
Should we consider adding new entries to ValueMappings so that we don't create this array locally? All the binary operations above don't create local arrays for this (though they could).


================
Comment at: llvm/lib/Target/RISCV/GISel/RISCVRegisterBankInfo.cpp:131
+        getGPRValueMapping(Ty2.getSizeInBits());
+    OperandsMapping = getOperandsMapping(
+        {GPRValueMapping, nullptr, GPRValueMapping, GPRValueMapping});
----------------
Should we consider adding new entries to ValueMappings so that we don't create this array locally? All the binary operations above don't create local arrays for this (though they could).


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu32.mir:1
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -march=riscv32 -run-pass=regbankselect -disable-gisel-legality-check -simplify-mir -verify-machineinstrs %s -o - \
----------------
Missing test for G_CONSTANT and G_ICMP


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu32.mir:6
+---
+name:            add_anyext
+legalized:       true
----------------
This test is identical to add_i32 below.


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu32.mir:51
+    %10:_(s32) = G_CONSTANT i32 24
+    %9:_(s32) = G_SHL %8, %10(s32)
+    %5:_(s32) = G_ASHR %9, %10(s32)
----------------
I don't think we need this test since we're separately testing for shl and ashr.


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu32.mir:78
+    %8:_(s32) = G_ADD %0, %2
+    %9:_(s32) = G_CONSTANT i32 255
+    %5:_(s32) = G_AND %8, %9
----------------
We don't have a test for G_CONSTANT, but if we make one, we may not need this test. We have tests for G_AND.


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu32.mir:286
+    %1:_(s32) = COPY $x11
+    %2:_(s32) = G_MUL %0, %1
+    $x10 = COPY %2(s32)
----------------
It's impossible to get a legal G_MUL on RISCV unless the +m or +zmmul attributes are enabled. It might be fine to have this test here to simply demonstrate how we select register banks for G_MUL.


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu32.mir:384
+---
+name:            add_i64
+legalized:       true
----------------
I'm not sure we need separate tests for _i64.


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu64.mir:1
+# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+# RUN: llc -march=riscv64 -run-pass=regbankselect -disable-gisel-legality-check -simplify-mir -verify-machineinstrs %s -o - \
----------------
Missing test for G_CONSTANT and G_ICMP


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu64.mir:6
+---
+name:            add_anyext
+legalized:       true
----------------
Duplicate test, see add_i64


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu64.mir:43
+    ; RV64I-NEXT: [[C:%[0-9]+]]:gprb(s64) = G_CONSTANT i64 56
+    ; RV64I-NEXT: [[SHL:%[0-9]+]]:gprb(s64) = G_SHL [[ADD]], [[C]](s64)
+    ; RV64I-NEXT: [[ASHR:%[0-9]+]]:gprb(s64) = G_ASHR [[SHL]], [[C]](s64)
----------------
I don't think we need this test since we're separately testing for shl and ashr.




================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu64.mir:78
+    %8:_(s64) = G_ADD %0, %2
+    %9:_(s64) = G_CONSTANT i64 255
+    %5:_(s64) = G_AND %8, %9
----------------
Once we have a test for G_CONSTANT, we may not need this test.


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu64.mir:85
+---
+name:            add_i32
+legalized:       true
----------------
All the _i32 tests are duplicates of the _i64 tests.


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/regbankselect/alu64.mir:664
+---
+name:            add_i128
+legalized:       true
----------------
I don't think that the _i128 tests are meaningful, since we test for each of these generic opcodes separately.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76051