Merge pull request #1617 from arrv-sc/master

workaround to support custom extensions that use standard prefixes

Author: aswaterman

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);