[PATCH] D137350: [RISCV] Implement assembler support for XVentanaCondOps

[Craig Topper via Phabricator via cfe-commits]

craig.topper added inline comments.


================
Comment at: clang/test/Preprocessor/riscv-target-features.c:437
+
+// RUN: %clang -target riscv64 -march=rv64ixventanacondops -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-XVENTANACONDOPS-EXT %s
----------------
reames wrote:
> Naming wise, is xventanacondops what we want this called?  The toolchain docs linked are a bit ambiguous on this point.  They seem to be saying that the extension should maybe be xvtcondops.  But that's not what the spec uses, not what we tend to use in discussion, and not what is being discussed for binutils.    
qemu seems to have also used xventanacondops.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:422
+    : SubtargetFeature<"xventanacondops", "HasVendorXVentanaCondOps", "true",
+                       "'XVentanaCondOps' (Ventana Conditional Move)">;
+def HasVendorXVentanaCondOps : Predicate<"Subtarget->hasVendorXVentanaCondOps()">,
----------------
It's not quite conditional move. It's conditional move or zero right? Might be better just says "Ops" or "Operations" instead of "Move".


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:426
+                                "'XVentanaCondOps' (Ventana Conditional Move)">;
+//===----------------------------------------------------------------------===//
+// LLVM specific features and extensions
----------------
Blank line above this.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrFormats.td:148
 def OPC_SYSTEM    : RISCVOpcode<"SYSTEM",    0b1110011>;
+def OPC_CUSTOM3   : RISCVOpcode<"CUSTOM3",   0b1111011>;
 
----------------
Since the string here is used for .insn parsing, I think it should be CUSTOM_3. I'm not sure why we don't already have all 4 added.


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

https://reviews.llvm.org/D137350