[offload] add nodiscard support to offload-tblgen and mark ol_errc_t

[311Volt]

Offload API functions may fail with error codes that shouldn't be ignored. Most notably, if olInit fails and its error return value is ignored, it is easy to use the library in an invalid uninitialized state, which can and has caused confusion. In those cases, it may be useful to have the ability to mark some API function with [[nodiscard]]

This PR adds an optional nodiscard property to offload-tblgen's Function, Enum, and Struct. If set, an OL_NODISCARD macro is emitted, which expands to [[nodiscard]] on >=C++17 and >=C23, and to nothing otherwise.

nodiscard is set for ol_errc_t, meaning every call to a function that returns it will emit a compiler warning if the return value is ignored and the TU is compiled on a supported language mode.

libsycl and llvm-gpu-loader still build cleanly and are unaffected by the change.

Worth considering: should there be an opt-out (#define OL_DISABLE_NODISCARD or similar)?

Assisted-by: Claude

[llvmorg-github-actions]

@llvm/pr-subscribers-offload

Author: 311Volt

Changes

Offload API functions may fail with error codes that shouldn't be ignored. Most notably, if olInit fails and its error return value is ignored, it is easy to use the library in an invalid uninitialized state, which can and has caused confusion. In those cases, it may be useful to have the ability to mark some API function with [[nodiscard]]

This PR adds an optional nodiscard property to offload-tblgen's Function. If set, an OL_NODISCARD macro is emitted, which expands to [[nodiscard]] on >=C++17 and >=C23, and to nothing otherwise.

Only olInit is initially marked nodiscard, but other functions - especially ones with output handles - are strong candidates to also mark nodiscard.

Worth considering: should there be an opt-out (#define OL_DISABLE_NODISCARD or similar)?

Assisted-by: Claude

---

Full diff: https://github.com/llvm/llvm-project/pull/209727.diff

5 Files Affected:

• (modified) offload/liboffload/API/APIDefs.td (+4)
• (modified) offload/liboffload/API/Common.td (+9)
• (modified) offload/test/tools/offload-tblgen/functions_basic.td (+30)
• (modified) offload/tools/offload-tblgen/APIGen.cpp (+5-2)
• (modified) offload/tools/offload-tblgen/RecordTypes.hpp (+1)

diff --git a/offload/liboffload/API/APIDefs.td b/offload/liboffload/API/APIDefs.td
index ea3896fc31035..db4e407df1164 100644
--- a/offload/liboffload/API/APIDefs.td
+++ b/offload/liboffload/API/APIDefs.td
@@ -154,6 +154,10 @@ class Function : APIObject {
   list<string> details = [];
   list<string> analogues = [];
 
+  // When set, the generated function declaration is marked with OL_NODISCARD
+  // so the compiler warns if the returned result is ignored.
+  bit nodiscard = 0;
+
   list<Return> returns_with_def = !listconcat(DefaultReturns, returns);
   list<Return> all_returns = AddPointerChecksToReturns<params,
         AddHandleChecksToReturns<params, returns_with_def>.returns_out>.returns_out;
diff --git a/offload/liboffload/API/Common.td b/offload/liboffload/API/Common.td
index 9bdd4291e096e..2e2adf94031a0 100644
--- a/offload/liboffload/API/Common.td
+++ b/offload/liboffload/API/Common.td
@@ -39,6 +39,14 @@ def OL_APIEXPORT : Macro {
   let alt_value = "";
 }
 
+def OL_NODISCARD : Macro {
+  let desc = "Marks a function whose result should not be discarded";
+  let condition = "(defined(__cplusplus) && __cplusplus >= 201703L) || "
+                  "(defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L)";
+  let value = "[[nodiscard]]";
+  let alt_value = "";
+}
+
 def ol_platform_handle_t : Handle {
   let desc = "Handle of a platform instance";
 }
@@ -155,6 +163,7 @@ def OL_INIT_ARGS_INIT : Macro {
 
 def olInit : Function {
   let desc = "Perform initialization of the Offload library";
+  let nodiscard = 1;
   let details = [
     "This must be the first API call made by a user of the Offload library",
     "Each call will increment an internal reference count that is decremented by `olShutDown`",
diff --git a/offload/test/tools/offload-tblgen/functions_basic.td b/offload/test/tools/offload-tblgen/functions_basic.td
index 2802c78a2947e..c7c6ab7f9b572 100644
--- a/offload/test/tools/offload-tblgen/functions_basic.td
+++ b/offload/test/tools/offload-tblgen/functions_basic.td
@@ -6,6 +6,14 @@
 
 include "APIDefs.td"
 
+def OL_NODISCARD : Macro {
+  let desc = "Marks a function whose result should not be discarded";
+  let condition = "(defined(__cplusplus) && __cplusplus >= 201703L) || "
+                  "(defined(__STDC_VERSION__) && __STDC_VERSION__ >= 202311L)";
+  let value = "[[nodiscard]]";
+  let alt_value = "";
+}
+
 def FunctionA : Function {
     let desc = "Function A description";
     let details = [ "Function A detailed information" ];
@@ -18,6 +26,20 @@ def FunctionA : Function {
     ];
 }
 
+// A function whose result should not be discarded.
+def FunctionB : Function {
+    let desc = "Function B description";
+    let nodiscard = 1;
+    let params = [
+        Param<"uint32_t", "ParamA", "Parameter A description">,
+    ];
+    let returns = [];
+}
+
+// The OL_NODISCARD macro is emitted as a conditional #define.
+// CHECK-API: #ifndef OL_NODISCARD
+// CHECK-API: #define OL_NODISCARD {{\[\[}}nodiscard{{\]\]}}
+
 // CHECK-API: /// @brief Function A description
 // CHECK-API: /// @details
 // CHECK-API-NEXT: Function A detailed information
@@ -25,6 +47,9 @@ def FunctionA : Function {
 // CHECK-API: OL_ERRC_INVALID_VALUE
 // CHECK-API-NEXT: When a value is invalid
 
+// FunctionA is not marked nodiscard, so its declaration and its WithCodeLoc
+// variant must not carry the attribute.
+// CHECK-API-NOT: OL_NODISCARD
 // CHECK-API: ol_result_t
 // CHECK-API-SAME: FunctionA
 
@@ -33,6 +58,11 @@ def FunctionA : Function {
 // CHECK-API: // Parameter B description
 // CHECK-API-NEXT: uint32_t* ParamB
 
+// FunctionB is marked nodiscard: both the primary declaration and the
+// WithCodeLoc variant are prefixed with OL_NODISCARD.
+// CHECK-API: OL_NODISCARD OL_APIEXPORT ol_result_t OL_APICALL FunctionB(
+// CHECK-API: OL_NODISCARD OL_APIEXPORT ol_result_t OL_APICALL FunctionBWithCodeLoc(
+
 // CHECK-EXPORTS: FunctionA
 
 // CHECK-FUNC-MACRO: OFFLOAD_FUNC(FunctionA)
diff --git a/offload/tools/offload-tblgen/APIGen.cpp b/offload/tools/offload-tblgen/APIGen.cpp
index 1e79c00ae06c5..7cd7fc264adfb 100644
--- a/offload/tools/offload-tblgen/APIGen.cpp
+++ b/offload/tools/offload-tblgen/APIGen.cpp
@@ -109,6 +109,8 @@ static void ProcessFunction(const FunctionRec &F, raw_ostream &OS) {
     }
   }
 
+  if (F.isNodiscard())
+    OS << "OL_NODISCARD ";
   OS << formatv("{0}_APIEXPORT {1}_result_t {0}_APICALL ", PrefixUpper,
                 PrefixLower);
   OS << F.getName();
@@ -212,9 +214,10 @@ static void ProcessFuncWithCodeLocVariant(const FunctionRec &Func,
 ///////////////////////////////////////////////////////////////////////////////
 /// @brief Variant of {0} that also sets source code location information
 /// @details See also ::{0}
-OL_APIEXPORT ol_result_t OL_APICALL {0}WithCodeLoc(
+{1}OL_APIEXPORT ol_result_t OL_APICALL {0}WithCodeLoc(
 )";
-  OS << formatv(FuncWithCodeLocBegin, Func.getName());
+  OS << formatv(FuncWithCodeLocBegin, Func.getName(),
+                Func.isNodiscard() ? "OL_NODISCARD " : "");
   auto Params = Func.getParams();
   for (auto &Param : Params) {
     OS << "  " << Param.getType() << " " << Param.getName();
diff --git a/offload/tools/offload-tblgen/RecordTypes.hpp b/offload/tools/offload-tblgen/RecordTypes.hpp
index 2abd9e10f0f96..16d8d8c50b6e9 100644
--- a/offload/tools/offload-tblgen/RecordTypes.hpp
+++ b/offload/tools/offload-tblgen/RecordTypes.hpp
@@ -225,6 +225,7 @@ class FunctionRec : public APIObject {
   std::vector<StringRef> getAnalogues() const {
     return rec->getValueAsListOfStrings("analogues");
   }
+  bool isNodiscard() const { return rec->getValueAsBit("nodiscard"); }
 
 private:
   std::vector<ReturnRec> rets;

[lplewa]

I have mix feeling about this change. I'm not sure if we should introduce things from newer standards conditionally. But I'm also not against this patch. But this is something I would like the maintainers to review.

@jdoerfert @jhuber6

[jhuber6]

To be fair, I think the HIP headers do this, but I'm unsure if it's worthwhile because these are supposed to be C headers and C23 is fairly new.

[311Volt]

Added nodiscard support to Enum and Struct and marked ol_errc_t as nodiscard, as proposed on the Wednesday meeting.

[jhuber6]

Is this definition guarded? Attributes are generally ignored, but it can create annoying warnings if it's unsupported. I couldn't find a check in the generated TD tests but I could be missing it.

[311Volt]

It's #if'd away when compiled on a language mode prior to [[nodiscard]] support (API/Common.td:42).

[lplewa]

@jhuber6 Do you have additional comments or i can merge this PR?