[PATCH] D76445: [RISCV][GlobalISel] Select ALU GPR instructions

[Nitin John Raj via Phabricator via llvm-commits]

nitinjohnraj added inline comments.


================
Comment at: llvm/lib/Target/RISCV/GISel/RISCVInstructionSelector.cpp:104
+  // TODO: Also check if we are seeing the result of an AND operation which
+  // could be bypassed since we only check the lower log2(xlen) bits.
+  return {{[=](MachineInstrBuilder &MIB) { MIB.add(Root); }}};
----------------
I assume we want to implement this TODO at higher optimization levels?


================
Comment at: llvm/lib/Target/RISCV/RISCVGISel.td:34
+// FIXME: Canonicalize (sub X, C) -> (add X, -C) earlier.
+def : Pat<(XLenVT (sub GPR:$rs1, simm12Plus1:$imm)),
+          (ADDI GPR:$rs1, (NegImm simm12Plus1:$imm))>;
----------------
Do we plan on implementing this earlier in a future patch? Why do we want to canonicalize it earlier?


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/alu-roundtrip.ll:36
+; RV64IM:       # %bb.0: # %entry
+; RV64IM-NEXT:    slli a0, a0, 24
+; RV64IM-NEXT:    sraiw a0, a0, 24
----------------
I think this should be slliw?


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/alu-roundtrip.ll:412
+; RV32IM-NEXT:    sltiu a2, a0, 1234
+; RV32IM-NEXT:    mv a1, a1
+; RV32IM-NEXT:    andi a2, a2, 1
----------------
Why is this instruction emitted? This looks like a nop.

We may want to handle this in a later pass.


================
Comment at: llvm/test/CodeGen/RISCV/GlobalISel/instruction-select/alu-rv32.mir:3
+# RUN: llc -march=riscv32 -run-pass=instruction-select -simplify-mir -verify-machineinstrs %s -o - \
+# RUN: | FileCheck -check-prefix=RV32I %s
+
----------------
Do we want tests for `and_i64`, `or_i64` and `xor_i64`? And immediate variants for these and `add_i64`, `sub_i64`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76445