kparzysz-quic commented on code in PR #12999:
URL: https://github.com/apache/tvm/pull/12999#discussion_r992368913


##########
src/runtime/hexagon/hexagon_buffer.cc:
##########
@@ -57,17 +57,30 @@ struct DDRAllocation : public Allocation {
 
 struct VTCMAllocation : public Allocation {
   VTCMAllocation(size_t nbytes, size_t alignment) : Allocation(nbytes, 
alignment) {
-    // TODO(HWE): Handle alignments greater than 2k
-    CHECK(alignment <= 0x800) << "VTCMAllocation called for invalid alignment";
-    if ((nbytes & 0x7FF) && ((alignment & 0x7FF) == 0)) {
-      // Caller has requested 2k alignment, but the size is not a multiple of 
2k
-      // Adjust size to be a multiple of 2k so that we will allocate from the 
front of the pool
+    // For simplicity, the current VTCM dynamic pool supports the following 
alignments: less than
+    // or equal to 128 (0x80), and 2k (0x800)
+    CHECK((alignment <= 0x80) || (alignment == 0x800))
+        << "VTCMAllocation called for invalid alignment " << alignment;
+
+    if ((alignment == 0x800) && (nbytes & 0x7FF)) {
+      // Caller has requested 2k alignment, but the size is not a multiple of 
2k.
+      // Adjust size to be a multiple of 2k so that we will allocate from the 
front of the pool.
       nbytes = nbytes >> 11;
       nbytes = nbytes << 11;
       nbytes += 0x800;

Review Comment:
   Side note---an alternative would be `nbytes = (nbytes + 0x7ff) & -0x800`, 
and it would work without the check if `nbytes & 0x7ff`.  Same below.



##########
tests/cpp-runtime/hexagon/hexagon_vtcm_pool_tests.cc:
##########
@@ -25,50 +25,66 @@ using namespace tvm::runtime;
 using namespace tvm::runtime::hexagon;
 
 class HexagonVtcmPoolTest : public ::testing::Test {
-  void SetUp() override { vtcm_pool = HexagonDeviceAPI::Global()->VtcmPool(); }
+  void SetUp() override {
+    vtcm_pool = HexagonDeviceAPI::Global()->VtcmPool();
+    max_bytes = vtcm_pool->TotalBytes();
+  }
   void TearDown() override {}
 
  public:
   HexagonVtcmPool* vtcm_pool;
+  size_t max_bytes;
+  size_t two_k_block = 2048;
+  size_t one_k_block = 1024;
+  size_t min_bytes = 128;
 };
 
 TEST_F(HexagonVtcmPoolTest, basic) {
   void* ptr;
-  size_t max_bytes = vtcm_pool->TotalBytes();
-  size_t two_k_block = 2048;
-  size_t one_k_block = 1024;
-  size_t one_byte_block = 1;
+  void* ptr2;
+
   ptr = vtcm_pool->Allocate(max_bytes);
+  CHECK((reinterpret_cast<size_t>(ptr) & 0x7FF) == 0)

Review Comment:
   Please use `uintptr_t` (or `intptr_t`) for integer representations of 
pointers (these types are exactly for that).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to