From 6ba4fe8889e1b2c7e738068c084e00ae396859e2 Mon Sep 17 00:00:00 2001
From: John Demme <john.demme@microsoft.com>
Date: Fri, 24 Jan 2025 15:58:46 +0000
Subject: [PATCH] Feedback from Morten

---
 .../ESI/runtime/cpp/include/esi/Services.h        |  6 ++++--
 lib/Dialect/ESI/runtime/cpp/lib/backends/Xrt.cpp  | 15 ++++++++++-----
 lib/Dialect/ESI/runtime/cpp/tools/esitester.cpp   |  2 +-
 3 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/lib/Dialect/ESI/runtime/cpp/include/esi/Services.h b/lib/Dialect/ESI/runtime/cpp/include/esi/Services.h
index 7170b52f3e94..cb871b618a96 100644
--- a/lib/Dialect/ESI/runtime/cpp/include/esi/Services.h
+++ b/lib/Dialect/ESI/runtime/cpp/include/esi/Services.h
@@ -210,8 +210,10 @@ class HostMem : public Service {
     operator void *() const { return getPtr(); }
     virtual std::size_t getSize() const = 0;
     /// Flush the memory region to ensure that the device sees the latest
-    /// contents. Recommended before DMA transactions, though not all platforms
-    /// require it.
+    /// contents. Because some platforms require it before DMA transactions, it
+    /// is recommended to call this before any DMA on all platforms. On
+    /// platforms which don't require it, it is a cheap no-op virtual method
+    /// call.
     virtual void flush() {}
   };
 
diff --git a/lib/Dialect/ESI/runtime/cpp/lib/backends/Xrt.cpp b/lib/Dialect/ESI/runtime/cpp/lib/backends/Xrt.cpp
index c9a6777e2a28..4be7186db880 100644
--- a/lib/Dialect/ESI/runtime/cpp/lib/backends/Xrt.cpp
+++ b/lib/Dialect/ESI/runtime/cpp/lib/backends/Xrt.cpp
@@ -62,11 +62,16 @@ struct esi::backends::xrt::XrtAccelerator::Impl {
     // Find memory group for the host.
     ::xrt::xclbin xcl(xclbin);
     std::optional<::xrt::xclbin::mem> host_mem;
-    for (auto mem : xcl.get_mems())
+    for (auto mem : xcl.get_mems()) {
       // The host memory is tagged with "HOST[0]". Memory type is wrong --
       // reports as DRAM rather than host memory so we can't filter on that.
-      if (mem.get_tag().starts_with("HOST"))
-        host_mem = mem;
+      if (mem.get_tag().starts_with("HOST")) {
+        if (host_mem.has_value())
+          throw std::runtime_error("Multiple host memories found in xclbin");
+        else
+          host_mem = mem;
+      }
+    }
     if (!host_mem)
       throw std::runtime_error("No host memory found in xclbin");
     memoryGroup = host_mem->get_index();
@@ -120,7 +125,7 @@ namespace {
 class XrtHostMem : public HostMem {
 public:
   XrtHostMem(::xrt::device &device, int32_t memoryGroup)
-      : device(device), memoryGroup(memoryGroup){};
+      : device(device), memoryGroup(memoryGroup) {};
 
   struct XrtHostMemRegion : public HostMemRegion {
     XrtHostMemRegion(::xrt::device &device, std::size_t size,
@@ -135,7 +140,7 @@ class XrtHostMem : public HostMem {
     /// the pointer the user application sees.
     virtual void *getDevicePtr() const override { return (void *)bo.address(); }
     virtual std::size_t getSize() const override { return bo.size(); }
-    /// It is recommended to use 'sync' to flush the caches before executing any
+    /// It is required to use 'sync' to flush the caches before executing any
     /// DMA.
     virtual void flush() override { bo.sync(XCL_BO_SYNC_BO_TO_DEVICE); }
 
diff --git a/lib/Dialect/ESI/runtime/cpp/tools/esitester.cpp b/lib/Dialect/ESI/runtime/cpp/tools/esitester.cpp
index dd3c967a98ee..68a9066c1ddd 100644
--- a/lib/Dialect/ESI/runtime/cpp/tools/esitester.cpp
+++ b/lib/Dialect/ESI/runtime/cpp/tools/esitester.cpp
@@ -126,7 +126,7 @@ void dmaTest(Accelerator *acc, esi::services::HostMem::HostMemRegion &region,
       dataPtr[0] = 0x12345678 << i;
       dataPtr[1] = 0xDEADBEEF << i;
       region.flush();
-      readMem->write(8, (uint64_t)devicePtr);
+      readMem->write(8, reinterpret_cast<uint64_t>(devicePtr));
 
       // Wait for the accelerator to read the correct value. Timeout and fail
       // after 10ms.