erwei-xilinx · Oct 25, 2026 · Oct 21, 2026
@@ -285,7 +285,7 @@ The symbol name gives the meaning of the patch site:
 
 ROCR applies these limits:
 
-- A kernel can have more than one PDI patch site, and the sites can name different PDI sections. For example, the control code from MLIR-AIR loads an empty PDI before and after the design runs. ROCR copies each different PDI into device memory one time for each kernel. Sites that name PDIs with the same contents use one copy.
+- A kernel can have more than one PDI patch site, and the sites can name different PDI sections. For example, the control code from MLIR-AIR loads an empty PDI before and after the design runs. ROCR copies each named *section* into device memory one time per kernel: sites naming the same section (resolved by name, see above) share that copy. Two sections are never merged by content, even when their bytes are identical — a kernel whose control code loads the same bytes from two separate sections (MLIR-AIR's empty-PDI-before and empty-PDI-after are two such sections) gets two device copies, one per section. An earlier version merged by content instead; that gave a correct first dispatch of the kernel and a wrong one on every repeat.
 - Each PDI patch site offset must not be 0, must be a multiple of 4, and `offset + 8` must not be more than the control-code size.
 - Each argument patch site offset must be a multiple of 4, and `offset + 12` must not be more than the control-code size.
 - An argument can have more than one patch site. An argument index can have no patch site.

@@ -333,6 +333,71 @@ TEST(AieElfParse, ParsesTwoPdisInOneKernel) {
   EXPECT_EQ(k.pdis[1].patch_offsets, std::vector<std::uint32_t>{RelocationOffset(image, 2)});
 }
 
+TEST(AieElfParse, KeepsIdenticalPdiSectionsSeparate) {
+  // Two sites can name sections whose bytes happen to be equal (MLIR-AIR's control code loads the
+  // same empty PDI both before and after the design, as two separate sections). Each site's section
+  // must still get its own entry and so its own device placement: a kernel that shared one
+  // placement across two identical-content sections dispatched correctly once and incorrectly on
+  // every dispatch after.
+  //
+  // Same setup as ParsesTwoPdisInOneKernel, except the renamed section is also pointed at .pdi.1's
+  // own bytes (same offset and size), so its content is identical to .pdi.1's by construction
+  // rather than merely by the spare section's original content happening to differ.
+  auto image = ReadFile(kElf);
+  if (image.empty()) GTEST_SKIP() << "full-ELF artifact not built";
+
+  std::map<std::string, rocr::AMD::aie_elf::Kernel> kernels;
+  std::string error;
+  ASSERT_EQ(ParseImage(image, &kernels, &error), HSA_STATUS_SUCCESS) << error;
+
+  const std::uint32_t alias_index = RenameSpareSection(image, ".pdi.2");
+  ASSERT_NE(alias_index, 0u) << "no spare section to rename";
+  const std::uint32_t pdi1_index = FindSection(image, ".pdi.1");
+  ASSERT_NE(pdi1_index, 0u);
+  Elf32_Shdr alias = SectionHeader(image, alias_index);
+  const Elf32_Shdr pdi1 = SectionHeader(image, pdi1_index);
+  alias.sh_offset = pdi1.sh_offset;
+  alias.sh_size = pdi1.sh_size;
+  SetSectionHeader(image, alias_index, alias);
+  ASSERT_EQ(SectionContents(image, ".pdi.2"), SectionContents(image, ".pdi.1"))
+      << "the alias must hold the same bytes, or this test proves nothing";
+
+  // Move and grow .dynstr so it can hold the new symbol name.
+  const std::uint32_t dynstr_index = FindSection(image, ".dynstr");
+  ASSERT_NE(dynstr_index, 0u);
+  Elf32_Shdr dynstr = SectionHeader(image, dynstr_index);
+  std::vector<std::uint8_t> strings(image.begin() + dynstr.sh_offset,
+                                    image.begin() + dynstr.sh_offset + dynstr.sh_size);
+  const std::uint32_t new_name_offset = static_cast<std::uint32_t>(strings.size());
+  strings.insert(strings.end(), std::begin(".pdi.2"), std::end(".pdi.2"));
+  dynstr.sh_offset = static_cast<Elf32_Off>(image.size());
+  dynstr.sh_size = static_cast<Elf32_Word>(strings.size());
+  image.insert(image.end(), strings.begin(), strings.end());
+  SetSectionHeader(image, dynstr_index, dynstr);
+
+  // Rename dynamic symbol 3 (the second kernel argument) to ".pdi.2". It already lives in the
+  // control-code section, which is what the reader requires of a patched symbol.
+  const std::uint32_t dynsym_index = FindSection(image, ".dynsym");
+  ASSERT_NE(dynsym_index, 0u);
+  const Elf32_Shdr dynsym = SectionHeader(image, dynsym_index);
+  ASSERT_GE(dynsym.sh_size, 4 * sizeof(Elf32_Sym));
+  std::memcpy(
+      image.data() + dynsym.sh_offset + 3 * sizeof(Elf32_Sym) + offsetof(Elf32_Sym, st_name),
+      &new_name_offset, sizeof(new_name_offset));
+
+  // Point the second argument relocation at it, as a PDI address.
+  ASSERT_TRUE(MutateRelocation(image, /*index=*/2, /*sym=*/3, kAddress64));
+
+  ASSERT_EQ(ParseImage(image, &kernels, &error), HSA_STATUS_SUCCESS) << error;
+  const auto& k = kernels.begin()->second;
+  ASSERT_EQ(k.pdis.size(), 2u);
+  EXPECT_EQ(k.pdis[0].bytes, SectionContents(image, ".pdi.1"));
+  EXPECT_EQ(k.pdis[0].patch_offsets, std::vector<std::uint32_t>{RelocationOffset(image, 0)});
+  EXPECT_EQ(k.pdis[1].bytes, SectionContents(image, ".pdi.2"));
+  EXPECT_EQ(k.pdis[1].patch_offsets, std::vector<std::uint32_t>{RelocationOffset(image, 2)});
+  EXPECT_EQ(k.pdis[0].bytes, k.pdis[1].bytes);
+}
+
 TEST(AieElfParse, RejectsAmbiguousPdiName) {
   // PDI sections are not group members, so a relocation names its PDI by section name alone. A
   // name shared by sections with different contents has no right answer, so it is refused rather

@@ -51,8 +51,19 @@ struct Kernel {
   std::string name;
   /// @brief The PDIs the control code loads; empty if it loads none. A control code may load
   /// several PDIs, or one more than once (MLIR-AIR's loads an empty PDI before and after the
-  /// design runs). Each distinct PDI appears once: sites naming byte-identical PDIs share an
-  /// entry, so it is placed in device memory once.
+  /// design runs). Entries are keyed by the ELF *section* a patch site names, not by byte
+  /// content: two sites naming the same section share one entry and one device placement, but
+  /// two different sections are placed separately even when their bytes are identical.
+  ///
+  /// This was not always true: an earlier version keyed on byte content instead, so two sections
+  /// with identical bytes (e.g. an empty PDI loaded both before and after the design) collapsed to
+  /// one placement at one device address. A kernel built that way dispatched correctly once and
+  /// incorrectly on every dispatch after. Nothing in this reader explains why -- it does not touch
+  /// the firmware or driver layer that actually loads a PDI at an address -- so this is recorded as
+  /// an observed fix, not a diagnosed root cause. Keying by section instead of content does not
+  /// conflict with anything the format document requires: it never asked for content-based merging
+  /// across distinct sections, only that sites naming the *same* section (by name, resolved once by
+  /// `find_pdi_section`) share a copy.
   std::vector<Pdi> pdis;
   /// @brief Control-code bytes.
   std::vector<uint8_t> ctrl_code;

@@ -11,6 +11,7 @@
 #include <algorithm>
 #include <cstring>
 #include <limits>
+#include <unordered_map>
 
 namespace rocr {
 namespace AMD {
@@ -343,6 +344,11 @@ hsa_status_t Parse(const void* image_data, size_t image_size, std::string_view a
 
     auto pdi_sites_it = group_pdi_sites.find(grp_index);
     if (pdi_sites_it != group_pdi_sites.end()) {
+      // Two sites sharing one Pdi entry (and so one device placement) are keyed by *section
+      // index*, not by content: see the Kernel::pdis doc comment for why two sections with
+      // identical bytes must not collapse into one placement, even though this is where that
+      // used to happen.
+      std::unordered_map<uint32_t, size_t> pdi_index_of_section;
       for (const auto& [pdi_section, offset] : pdi_sites_it->second) {
         // A 64-bit address is written here, so it must lie wholly inside the control code.
         if (offset + sizeof(uint64_t) > k.ctrl_code.size() || offset % sizeof(uint32_t) != 0) {
@@ -353,13 +359,15 @@ hsa_status_t Parse(const void* image_data, size_t image_size, std::string_view a
         if (offset == 0) {
           return fail("PDI patch site at offset 0 is indistinguishable from no patch");
         }
-        std::vector<uint8_t> bytes;
-        if (!section_bytes(pdi_section, &bytes)) return fail("section extends past end of file");
-        // Each distinct PDI is placed in device memory once, however many sites load it.
-        auto pdi = std::find_if(k.pdis.begin(), k.pdis.end(),
-                                [&](const Pdi& p) { return p.bytes == bytes; });
-        if (pdi == k.pdis.end()) pdi = k.pdis.insert(k.pdis.end(), Pdi{std::move(bytes), {}});
-        pdi->patch_offsets.push_back(offset);
+        auto [it, inserted] = pdi_index_of_section.try_emplace(pdi_section, k.pdis.size());
+        if (inserted) {
+          std::vector<uint8_t> bytes;
+          if (!section_bytes(pdi_section, &bytes)) {
+            return fail("section extends past end of file");
+          }
+          k.pdis.push_back(Pdi{std::move(bytes), {}});
+        }
+        k.pdis[it->second].patch_offsets.push_back(offset);
       }
     }