[PATCH] D143036: [RISCV] Add vendor-defined XTHeadBs (single-bit) extension

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

This generally looks reasonable to me, and I support adding this vendor extension once normal code review is complete.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:1243
+      Subtarget.hasStdExtZbs() && X.getValueType().isScalarInteger();
   auto *C = dyn_cast<ConstantSDNode>(Y);
+  // XTheadBs provides th.tst (similar to bexti), if Y is a constant
----------------
Style wise, a series of early returns here would be easy to follow.  


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:80
+let IsSignExtendingOpW = 1 in
+def TH_TST : RVBShift_ri<0b10001, 0b001, OPC_CUSTOM_0, "th.tst">,
+             Sched<[WriteSingleBitImm, ReadSingleBitImm]>;
----------------
Missing decoder namerspace


================
Comment at: llvm/test/CodeGen/RISCV/rv32xtheadbs.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
----------------
In this diff, you have coverage for both 32 and 64, but is this extension actually defined for RV32?  The previous diff (THeadBa) was only tested on RV64.  Why are these two handled differently here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143036