[PATCH] D95638: AMDGPU: Add target id and code object v4 support

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Apologies for not coming back to review this before. I've taken a look through the llvm-readobj changes, and highlighted a number of code paths which I don't think have been tested at all (based on the fact that strings that should be printed don't appear in any of the test changes with this patch). Please create a new patch to address these and my previous comment. If necessary, use yaml2obj to generate note sections with the appropriate metadata.



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5007-5009
+    if (Desc.size() != sizeof(CodeObjectVersion))
+      return {"AMD HSA Code Object Version",
+              "Invalid AMD HSA Code Object Version"};
----------------
This code appears to be untested.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5013-5014
+    auto Version = reinterpret_cast<const CodeObjectVersion *>(Desc.data());
+    StrOS << "[Major: " << Version->MajorVersion
+          << ", Minor: " << Version->MinorVersion << "]";
+    return {"AMD HSA Code Object Version", VersionString};
----------------
This code is untested.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5025-5026
+    };
+    if (Desc.size() != sizeof(HSAILProperties))
+      return {"AMD HSA HSAIL Properties", "Invalid AMD HSA HSAIL Properties"};
+    auto Properties = reinterpret_cast<const HSAILProperties *>(Desc.data());
----------------
This code appears to be untested.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5030-5034
+    StrOS << "[HSAIL Major: " << Properties->HSAILMajorVersion
+          << ", HSAIL Minor: " << Properties->HSAILMinorVersion
+          << ", Profile: " << Properties->Profile
+          << ", Machine Model: " << Properties->MachineModel
+          << ", Default Float Round: " << Properties->DefaultFloatRound << "]";
----------------
This code is untested.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5045-5046
+    };
+    if (Desc.size() < sizeof(IsaVersion))
+      return {"AMD HSA ISA Version", "Invalid AMD HSA ISA Version"};
+    auto Isa = reinterpret_cast<const IsaVersion *>(Desc.data());
----------------
This code appears to be untested.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5048-5051
+    if (Desc.size() < sizeof(IsaVersion) +
+                          Isa->VendorNameSize + Isa->ArchitectureNameSize ||
+        Isa->VendorNameSize == 0 || Isa->ArchitectureNameSize == 0)
+      return {"AMD HSA ISA Version", "Invalid AMD HSA ISA Version"};
----------------
This code appears to be untested.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5054-5060
+    StrOS << "[Vendor: "
+          << StringRef((const char*)Desc.data() + sizeof(IsaVersion), Isa->VendorNameSize - 1)
+          << ", Architecture: "
+          << StringRef((const char*)Desc.data() + sizeof(IsaVersion) + Isa->VendorNameSize,
+                       Isa->ArchitectureNameSize - 1)
+          << ", Major: " << Isa->Major << ", Minor: " << Isa->Minor
+          << ", Stepping: " << Isa->Stepping << "]";
----------------
This code is untested.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95638/new/

https://reviews.llvm.org/D95638