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

[Philip Reames via Phabricator via llvm-commits]

reames 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
----------------
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.    


================
Comment at: clang/test/Preprocessor/riscv-target-features.c:439
+// RUN: -o - | FileCheck --check-prefix=CHECK-XVENTANACONDOPS-EXT %s
+// CHECK-XVENTANACONDOPS-EXT: __riscv_xventanacondops 1000000{{$}}
----------------
This patch only includes positive tests for riscv64.  Per the current specification, no rv32 chips have shipped with this feature.  I chose not to actually reject the flags or elf settings for riscv32, but to not enable the assembler either.

What do we think is the right answer here?  Should we accept the riscv32 forms?  This would essentially be an LLVM extension.  Should we explicitly error on the riscv32 form?  Something else?


================
Comment at: llvm/docs/RISCVUsage.rst:161
+``XVentanaCondOps``
+  LLVM implements `version 1.0.0 of the VTx-family custom instructions specification <https://github.com/ventanamicro/ventana-custom-extensions/releases/download/v1.0.0/ventana-custom-extensions-v1.0.0.pdf>`_ by Ventana Micro Systems.  All instructions are prefixed with `vt.` as described in the specification, and the riscv-toolchai-convention document linked above.  These instructions are only available for riscv64 at this time.
+
----------------
Anything else we want in the documentation?  Remember, we're setting precedent here.  


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1785
+//===----------------------------------------------------------------------===//
+// Vendor extensions
+//===----------------------------------------------------------------------===//
----------------
How do we want to manage td files for vendor extensions?

I put them inline - which is probably not the right answer.  I'm leaning towards a vendor specific td file with extensions split out if complexity justifies it.  So, this patch would add a RISCVInstInfoXVentana.td.  

Is that the precedent we want here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137350