[PATCH] D127776: [ObjectYAML] Add offloading binary implementations for obj2yaml and yaml2obj

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Have you got any reference material that I can read for the format of offloading binaries? I can't really review that this implementation does the right thing without knowing the format!



================
Comment at: llvm/lib/ObjectYAML/OffloadYAML.cpp:26
+  IO.mapTag("!Offload", true);
+  IO.mapOptional("Members", O.Members);
+  IO.setContext(nullptr);
----------------
Here and with all the other fields, you need test cases that show that, if they're optional, the fields can be omitted (and what their default values are if so), and if they're required, an error is emitted if the field is missing.


================
Comment at: llvm/lib/ObjectYAML/OffloadYAML.cpp:51
+      (object::getOffloadKind(*M.OffloadKind) == object::OFK_None)) {
+    return "Invalid offload kind provided";
+  }
----------------
Here and below, these error messages should start with a lower-case letter, I suspect (see the Coding Standards for error message format).

That being said, in ELF yaml2obj, we allow specification of values that may not be permitted by the spec but could theoretically appear in the field, if something wrote the wrong value. This allows for testing of error paths more effectively. I can't give further pointers though without knowing the format and what makes sense to allow to handcraft.


================
Comment at: llvm/test/ObjectYAML/Offload/multiple_members.yaml:1
+# RUN: yaml2obj %s | obj2yaml | FileCheck %s --strict-whitespace
+!Offload
----------------
I don't think you need `--strict-whitespace` as you're not really interested in the whitespace in this test.

I'm not sure there's much benefit in having a single and a multiple entry test case. I'd just have the multiple test case, and drop the single one.


================
Comment at: llvm/test/ObjectYAML/Offload/multiple_members.yaml:12
+      Value:            "sm_70"
+    Content:          "deadbeef"  
+  - ImageKind:        "bc"
----------------
Let's use a different Content for the two members. It avoids any risk of a content field being cached from one member to the other.


================
Comment at: llvm/tools/obj2yaml/offload2yaml.cpp:1
+//===------ utils/offload2yaml.cpp - obj2yaml conversion tool ---*- C++ -*-===//
+//
----------------
This file isn't in "utils", so the header tag here is wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127776