Address review comments

Author: sherry-yuan

include/acl.h

@@ -480,11 +480,10 @@ typedef class acl_device_program_info_t *acl_device_program_info;
 #define ACL_MEM_CAPABILITY_P2P (1 << 3)
 
 // Definition of device global.
-typedef struct acl_device_global_mem_def_t {
-  std::string name;
+struct acl_device_global_mem_def_t {
   unsigned int address;
-  unsigned int size;
-} acl_device_global_mem_def_t;
+  uint32_t size;
+};
 
 // Part of acl_device_def_t where members are populated from the information
 // in the autodiscovery string. This will get updated every time the device

src/acl_auto_configure.cpp

@@ -99,6 +99,25 @@ static bool read_uint_counters(const std::string &str,
   return true;
 }
 
+// Reads the next word in str and converts it into an unsigned.
+// Returns true if a valid integer was read or false if an error occurred.
+// pos is updated to the position immediately following the parsed word
+// even if an error occurs.
+static bool read_uint32_counters(const std::string &str,
+                                 std::string::size_type &pos, uint32_t &val,
+                                 std::vector<int> &counters) noexcept {
+  std::string result;
+  pos = read_word(str, pos, result);
+  decrement_section_counters(counters);
+  try {
+    val = static_cast<uint32_t>(std::stoul(result));
+  } catch (const std::exception &e) {
+    UNREFERENCED_PARAMETER(e);
+    return false;
+  }
+  return true;
+}
+
 // Reads the next word in str and converts it into an unsigned.
 // Returns true if a valid integer was read or false if an error occurred.
 // pos is updated to the position immediately following the parsed word
@@ -501,10 +520,10 @@ bool acl_load_device_def_from_str(const std::string &config_str,
     devdef.num_device_global = num_device_global;
 
     // read total number of fields in device global
-    int total_fields_device_global = 0;
+    unsigned int total_fields_device_global = 0;
     if (result) {
-      result = read_int_counters(config_str, curr_pos,
-                                 total_fields_device_global, counters);
+      result = read_uint_counters(config_str, curr_pos,
+                                  total_fields_device_global, counters);
     }
 
     for (auto i = 0U; result && (i < num_device_global);
@@ -525,18 +544,27 @@ bool acl_load_device_def_from_str(const std::string &config_str,
             read_uint_counters(config_str, curr_pos, dev_global_addr, counters);
       }
       // read device global address size
-      unsigned int dev_global_size = 0; // Default
+      uint32_t dev_global_size = 0; // Default
       if (result && counters.back() > 0) {
-        result =
-            read_uint_counters(config_str, curr_pos, dev_global_size, counters);
+        result = read_uint32_counters(config_str, curr_pos, dev_global_size,
+                                      counters);
       }
 
-      acl_device_global_mem_def_t dev_global_def = {
-          device_global_name, dev_global_addr, dev_global_size};
-      devdef.device_global_mem_defs[device_global_name] = dev_global_def;
+      acl_device_global_mem_def_t dev_global_def = {dev_global_addr,
+                                                    dev_global_size};
+      bool ok = devdef.device_global_mem_defs
+                    .insert({device_global_name, dev_global_def})
+                    .second;
+      if (!ok) {
+        // Device global name already exist in map, but it should have been
+        // unique.
+        err_ss << "Device global name should be unique. " << device_global_name
+               << " is repeated.\n";
+        result = false;
+      }
 
-      // forward compatibility: bypassing remaining fields at the end of global
-      // memory
+      // forward compatibility: bypassing remaining fields at the end of device
+      // global memory
       while (result && counters.size() > 0 &&
              counters.back() > 0) { // total_fields_device_global>0
         std::string tmp;

test/acl_auto_configure_test.cpp

@@ -270,25 +270,25 @@ TEST(auto_configure, simple) {
   CHECK_EQUAL(1, (int)m_device_def.autodiscovery_def.accel[0].is_sycl_compile);
 
   // Checks for device global entry.
-  CHECK_EQUAL(2, (int)m_device_def.autodiscovery_def.num_device_global);
+  CHECK_EQUAL(2, m_device_def.autodiscovery_def.num_device_global);
   CHECK(m_device_def.autodiscovery_def.device_global_mem_defs.find(
             "kernel15_dev_global") !=
         m_device_def.autodiscovery_def.device_global_mem_defs.end());
   CHECK(m_device_def.autodiscovery_def.device_global_mem_defs.find(
             "kernel15_dev_global2") !=
         m_device_def.autodiscovery_def.device_global_mem_defs.end());
-  CHECK_EQUAL(4096, m_device_def.autodiscovery_def
-                        .device_global_mem_defs["kernel15_dev_global"]
-                        .address);
-  CHECK_EQUAL(2048, m_device_def.autodiscovery_def
-                        .device_global_mem_defs["kernel15_dev_global"]
-                        .size);
-  CHECK_EQUAL(2048, m_device_def.autodiscovery_def
-                        .device_global_mem_defs["kernel15_dev_global2"]
-                        .address);
-  CHECK_EQUAL(1024, m_device_def.autodiscovery_def
-                        .device_global_mem_defs["kernel15_dev_global2"]
-                        .size);
+  CHECK_EQUAL(4096, m_device_def.autodiscovery_def.device_global_mem_defs
+                        .find("kernel15_dev_global")
+                        ->second.address);
+  CHECK_EQUAL(2048, m_device_def.autodiscovery_def.device_global_mem_defs
+                        .find("kernel15_dev_global")
+                        ->second.size);
+  CHECK_EQUAL(2048, m_device_def.autodiscovery_def.device_global_mem_defs
+                        .find("kernel15_dev_global2")
+                        ->second.address);
+  CHECK_EQUAL(1024, m_device_def.autodiscovery_def.device_global_mem_defs
+                        .find("kernel15_dev_global2")
+                        ->second.size);
 
   // Check a second parsing.
   // It should allocate a new string for the name.
@@ -490,7 +490,7 @@ TEST(auto_configure, many_ok_forward_compatibility) {
                                     "sample40byterandomhash000000000000000000 "
                                     "a10gx 0 1 15 DDR 2 1 6 0 2147483648 100 "
                                     "100 100 100 200 200 200 200 0 0 0 0 2 "
-                                    "1 1 1 400 47 "
+                                    "1 name1 1 name2 47 "
                                     "40 external_sort_stage_0 0 128 1 0 0 1 0 "
                                     "1 0 1 10 0 0 4 1 0 0 500 500 500 500 0 0 "
                                     "0 0 1 1 1 3 1 1 1 3 1 800 800 800 800 800 "
@@ -1204,7 +1204,7 @@ TEST(auto_configure, hostpipe) {
                                     "200 "
                                     "2 9 host_to_dev 1 0 32 32768 300 300 300 "
                                     "300 dev_to_host 0 1 32 32768 300 300 300 "
-                                    "300 400 1 3 400 400 0 "
+                                    "300 400 1 3 name3 400 0 "
                                     "1 29 foo 0 128 1 0 0 1 0 1 0 0 0 0 0 0 1 "
                                     "1 1 3 1 1 1 3 1 800 800 800 800 800 900 "
                                     "900"