workaround to support custom extensions that use standard prefixes

RISC-V ISA states (21.1):
"A standard-compatible global encoding can also use standard prefixes
for non-standard extensions if the associated standard extensions are
not included in the global encoding."

Currently all the instructions (either from standard or custom
extensions) are all being inserted into a single std::vector which is
then being sorted. An instruction matching process performs linear
search on that vector. The problem is that when a custom extension uses
the same opcode as standard one (i.e. match and mask are equal to the
standard counterparts) it is undefined which instruction will be picked.
That is because in std::sort "The order of equal elements is not
guaranteed to be preserved". That being said it is impossible to define
custom extension (via customext) that would use the prefix of a disabled
standard extension.

In this change I separate custom and standard extensions in two separate
std::vector's. By default we report an error if they have common
elements (There're an additional processor_t constructor's argument that
skips this check). If this error is disabled during instruction matching
we first trying to find it among custom instructions. If it has been
found the search is stopped and custom instruction is executed,
otherwise we look for it among standard instructions. Overall this
change does not completely fix the problem but at least makes it
possible to use the feature of RISC-V ISA.

Author: arrv-sc

ci-tests/create-ci-binary-tarball

@@ -12,9 +12,13 @@ mkdir -p build/hello && cd "$_"
 riscv64-unknown-elf-gcc -O2 -o hello `git rev-parse --show-toplevel`/ci-tests/hello.c
 cd -
 
+mkdir -p build/dummy-slliuw && cd "$_"
+riscv64-unknown-elf-gcc -O2 -o dummy-slliuw `git rev-parse --show-toplevel`/ci-tests/dummy-slliuw.c
+cd -
+
 mv build/pk/pk .
 mv build/hello/hello .
+mv build/dummy-slliuw/dummy-slliuw .
+tar -cf spike-ci.tar pk hello dummy-slliuw
 
-tar -cf spike-ci.tar pk hello
-
-rm pk hello
+rm pk hello dummy-slliuw

ci-tests/dummy-slliuw.c

@@ -0,0 +1,8 @@
+#include <stdio.h>
+
+int main() {
+  // as if slli.uw zero, t1, 3
+  asm(".4byte 0x0833101b");
+  printf("Executed successfully\n");
+  return 0;
+}

ci-tests/test-customext.cc

@@ -0,0 +1,90 @@
+#include <riscv/extension.h>
+#include <riscv/sim.h>
+
+struct : public arg_t {
+  std::string to_string(insn_t insn) const { return xpr_name[insn.rd()]; }
+} xrd;
+
+struct : public arg_t {
+  std::string to_string(insn_t insn) const { return xpr_name[insn.rs1()]; }
+} xrs1;
+
+struct : public arg_t {
+  std::string to_string(insn_t insn) const { return xpr_name[insn.rs2()]; }
+} xrs2;
+
+struct : public arg_t {
+  std::string to_string(insn_t insn) const {
+    return std::to_string((int)insn.shamt());
+  }
+} shamt;
+
+static reg_t do_nop4([[maybe_unused]] processor_t *p,
+                     [[maybe_unused]] insn_t insn, reg_t pc) {
+  return pc + 4;
+}
+
+// dummy extension that uses the same prefix as standard zba extension
+struct xslliuw_dummy_t : public extension_t {
+  const char *name() { return "dummyslliuw"; }
+
+  xslliuw_dummy_t() {}
+
+  std::vector<insn_desc_t> get_instructions() {
+    std::vector<insn_desc_t> insns;
+    insns.push_back(insn_desc_t{MATCH_SLLI_UW, MASK_SLLI_UW, do_nop4, do_nop4,
+                                do_nop4, do_nop4, do_nop4, do_nop4, do_nop4,
+                                do_nop4});
+    return insns;
+  }
+
+  std::vector<disasm_insn_t *> get_disasms() {
+    std::vector<disasm_insn_t *> insns;
+    insns.push_back(new disasm_insn_t("dummy_slliuw", MATCH_SLLI_UW,
+                                      MASK_SLLI_UW, {&xrd, &xrs1, &shamt}));
+    return insns;
+  }
+};
+
+REGISTER_EXTENSION(dummyslliuw, []() { return new xslliuw_dummy_t; })
+
+// Copied from spike main.
+// TODO: This should really be provided in libriscv
+static std::vector<std::pair<reg_t, abstract_mem_t *>>
+make_mems(const std::vector<mem_cfg_t> &layout) {
+  std::vector<std::pair<reg_t, abstract_mem_t *>> mems;
+  mems.reserve(layout.size());
+  for (const auto &cfg : layout) {
+    mems.push_back(std::make_pair(cfg.get_base(), new mem_t(cfg.get_size())));
+  }
+  return mems;
+}
+
+int main(int argc, char **argv) {
+  cfg_t cfg;
+  cfg.isa = "RV64IMAFDCV_xdummyslliuw";
+  std::vector<device_factory_t *> plugin_devices;
+  if (argc != 3) {
+    std::cerr << "Error: invalid arguments\n";
+    exit(1);
+  }
+  std::vector<std::string> htif_args{argv[1] /* pk */, argv[2] /* executable */};
+  debug_module_config_t dm_config = {.progbufsize = 2,
+                                     .max_sba_data_width = 0,
+                                     .require_authentication = false,
+                                     .abstract_rti = 0,
+                                     .support_hasel = true,
+                                     .support_abstract_csr_access = true,
+                                     .support_abstract_fpr_access = true,
+                                     .support_haltgroups = true,
+                                     .support_impebreak = true};
+  std::vector<std::pair<reg_t, abstract_mem_t *>> mems =
+      make_mems(cfg.mem_layout);
+  sim_t sim(&cfg, false, mems, plugin_devices, htif_args, dm_config,
+            nullptr,  // log_path
+            true,     // dtb_enabled
+            nullptr,  // dtb_file
+            false,    // socket_enabled
+            nullptr); // cmd_file
+  sim.run();
+}

ci-tests/test-spike

@@ -14,4 +14,7 @@ time ../install/bin/spike --isa=rv64gc pk hello | grep "Hello, world!  Pi is app
 
 # check that including sim.h in an external project works
 g++ -std=c++17 -I../install/include -L../install/lib $DIR/testlib.c -lriscv -o test-libriscv
-LD_LIBRARY_PATH=../install/lib ./test-libriscv | grep "Hello, world!  Pi is approximately 3.141588."
+g++ -std=c++17 -I../install/include -L../install/lib $DIR/test-customext.cc -lriscv -o test-customext
+
+LD_LIBRARY_PATH=../install/lib ./test-libriscv pk hello| grep "Hello, world!  Pi is approximately 3.141588."
+LD_LIBRARY_PATH=../install/lib ./test-customext pk dummy-slliuw | grep "Executed successfully"

ci-tests/testlib.c

@@ -12,11 +12,16 @@ static std::vector<std::pair<reg_t, abstract_mem_t*>> make_mems(const std::vecto
   return mems;
 }
 
-int main()
-{
+int main(int argc, char **argv) {
   cfg_t cfg;
   std::vector<device_factory_t*> plugin_devices;
-  std::vector<std::string> htif_args {"pk", "hello"};
+
+  if (argc != 3) {
+    std::cerr << "Error: invalid arguments\n";
+    exit(1);
+  }
+  std::vector<std::string> htif_args{argv[1] /* pk */,
+                                     argv[2] /* executable */};
   debug_module_config_t dm_config;
   std::vector<std::pair<reg_t, abstract_mem_t*>> mems =
       make_mems(cfg.mem_layout);

riscv/processor.cc

@@ -1037,6 +1037,24 @@ reg_t illegal_instruction(processor_t UNUSED *p, insn_t insn, reg_t UNUSED pc)
   throw trap_illegal_instruction(insn.bits() & 0xffffffffULL);
 }
 
+static insn_desc_t
+propagate_instruction_in_vector(std::vector<insn_desc_t> &instructions,
+                                std::vector<insn_desc_t>::iterator it) {
+  assert(it != instructions.end());
+  insn_desc_t desc = *it;
+  if (it->mask != 0 && it != instructions.begin() &&
+      std::next(it) != instructions.end()) {
+    if (it->match != std::prev(it)->match &&
+        it->match != std::next(it)->match) {
+      // move to front of opcode list to reduce miss penalty
+      while (--it >= instructions.begin())
+        *std::next(it) = *it;
+      instructions[0] = desc;
+    }
+  }
+  return desc;
+}
+
 insn_func_t processor_t::decode_insn(insn_t insn)
 {
   // look up opcode in hash table
@@ -1047,34 +1065,33 @@ insn_func_t processor_t::decode_insn(insn_t insn)
 
   if (unlikely(insn.bits() != desc.match)) {
     // fall back to linear search
-    int cnt = 0;
-    insn_desc_t* p = &instructions[0];
-    while ((insn.bits() & p->mask) != p->match)
-      p++, cnt++;
-    desc = *p;
-
-    if (p->mask != 0 && p > &instructions[0]) {
-      if (p->match != (p - 1)->match && p->match != (p + 1)->match) {
-        // move to front of opcode list to reduce miss penalty
-        while (--p >= &instructions[0])
-          *(p + 1) = *p;
-        instructions[0] = desc;
-      }
+    auto matching = [insn_bits = insn.bits()](const insn_desc_t &d) {
+      return (insn_bits & d.mask) == d.match;
+    };
+    auto p = std::find_if(custom_instructions.begin(),
+                          custom_instructions.end(), matching);
+    if (p != custom_instructions.end()) {
+      desc = propagate_instruction_in_vector(custom_instructions, p);
+    } else {
+      p = std::find_if(instructions.begin(), instructions.end(), matching);
+      assert(p != instructions.end());
+      desc = propagate_instruction_in_vector(instructions, p);
     }
-
     opcode_cache[idx] = desc;
     opcode_cache[idx].match = insn.bits();
   }
 
   return desc.func(xlen, rve, log_commits_enabled);
 }
 
-void processor_t::register_insn(insn_desc_t desc)
-{
+void processor_t::register_insn(insn_desc_t desc, bool is_custom) {
   assert(desc.fast_rv32i && desc.fast_rv64i && desc.fast_rv32e && desc.fast_rv64e &&
          desc.logged_rv32i && desc.logged_rv64i && desc.logged_rv32e && desc.logged_rv64e);
 
-  instructions.push_back(desc);
+  if (is_custom)
+    custom_instructions.push_back(desc);
+  else
+    instructions.push_back(desc);
 }
 
 void processor_t::build_opcode_map()
@@ -1086,16 +1103,17 @@ void processor_t::build_opcode_map()
       return lhs.match > rhs.match;
     }
   };
+
   std::sort(instructions.begin(), instructions.end(), cmp());
+  std::sort(custom_instructions.begin(), custom_instructions.end(), cmp());
 
   for (size_t i = 0; i < OPCODE_CACHE_SIZE; i++)
     opcode_cache[i] = insn_desc_t::illegal();
 }
 
-void processor_t::register_extension(extension_t* x)
-{
+void processor_t::register_extension(extension_t *x) {
   for (auto insn : x->get_instructions())
-    register_insn(insn);
+    register_custom_insn(insn);
   build_opcode_map();
 
   for (auto disasm_insn : x->get_disasms())
@@ -1131,7 +1149,7 @@ void processor_t::register_base_instructions()
     extern reg_t logged_rv32e_##name(processor_t*, insn_t, reg_t); \
     extern reg_t logged_rv64e_##name(processor_t*, insn_t, reg_t); \
     if (name##_supported) { \
-      register_insn((insn_desc_t) { \
+      register_base_insn((insn_desc_t) { \
         name##_match, \
         name##_mask, \
         fast_rv32i_##name, \
@@ -1144,10 +1162,8 @@ void processor_t::register_base_instructions()
         logged_rv64e_##name}); \
     }
   #include "insn_list.h"
-  #undef DEFINE_INSN
-
   // terminate instruction list with a catch-all
-  register_insn(insn_desc_t::illegal());
+  register_base_insn(insn_desc_t::illegal());
 
   build_opcode_map();
 }

riscv/processor.h

@@ -281,7 +281,12 @@ class processor_t : public abstract_device_t
 
   FILE *get_log_file() { return log_file; }
 
-  void register_insn(insn_desc_t);
+  void register_base_insn(insn_desc_t insn) {
+    register_insn(insn, false /* is_custom */);
+  }
+  void register_custom_insn(insn_desc_t insn) {
+    register_insn(insn, true /* is_custom */);
+  }
   void register_extension(extension_t*);
 
   // MMIO slave interface
@@ -336,6 +341,7 @@ class processor_t : public abstract_device_t
   mutable std::bitset<NUM_ISA_EXTENSIONS> extension_assumed_const;
 
   std::vector<insn_desc_t> instructions;
+  std::vector<insn_desc_t> custom_instructions;
   std::unordered_map<reg_t,uint64_t> pc_histogram;
 
   static const size_t OPCODE_CACHE_SIZE = 8191;
@@ -346,6 +352,7 @@ class processor_t : public abstract_device_t
   void take_trap(trap_t& t, reg_t epc); // take an exception
   void take_trigger_action(triggers::action_t action, reg_t breakpoint_tval, reg_t epc, bool virt);
   void disasm(insn_t insn); // disassemble and print an instruction
+  void register_insn(insn_desc_t, bool);
   int paddr_bits();
 
   void enter_debug_mode(uint8_t cause);