[PATCH] D139394: [RISCV] Add support for RISCV XVentanaCondops Extension

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4421
 
+  // Lower the Op just as it is without any changes.
+  // This will resolve this to Ventana's VT_MASKC/VT_MASKCN instruction
----------------
This doesn't belong here. It should be done on setOperationAction call that made ISD::SELECT Custom.

I think it's also incorrect here because it disabled SELECT_CC formation for FP selects.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXVentana.td:58
+def: Pat<(select i64:$rc, i64:$rs1, i64:$rs2),
+         (OR (VT_MASKC $rs1, $rc),(VT_MASKCN $rs2, $rc))>;
----------------
Missing space before `(VT_MASKCN`


================
Comment at: llvm/test/CodeGen/RISCV/xventanacondops.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
+; RUN: llc -mtriple=riscv64 -mattr=+xventanacondops -stop-after finalize-isel < %s | FileCheck %s -check-prefix=RV64
+
----------------
Why stop-after finalize-isel?


================
Comment at: llvm/test/MC/RISCV/rv64xventanacondops-valid.s:1
+# With XVentanacondops base extension:
+# RUN: llvm-mc %s -triple=riscv64 -mattr=+xventanacondops -show-encoding \
----------------
don't we already have a test called XVentanaCondOps-valid.s?


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

https://reviews.llvm.org/D139394