sprint(rocprofiler-compute): Q4 Sprint 1

projects/rocprofiler-compute/.ai/rules/cli-options.md

@@ -37,7 +37,7 @@ names since customers rarely touch them.
 A list of values is passed as one comma-separated argument.
 
 ```console
---roofline-data-types FP16,FP32,FP64
+--example-ids 0,1,2
 ```
 
 ## Help Messages
@@ -56,8 +56,8 @@ option description, starting with `Values: `. An argument with a default states
 it at the end of the description as `(Default: value)`.
 
 ```text
---roofline-data-types <types>...   Selects data <type>s to present in roofline (Default: FP32).
-                                    Values: FP4, FP6, FP8, FP16, BF16, FP32, FP64, I8, I32, I64.
---list-blocks [arch]               List all available blocks for analysis on specified GPU <arch> (Default: current GPU arch).
-                                    Values: gfx908, gfx90a, gfx940, gfx941, gfx942, gfx950, gfx1151
+--output-format <format>   Specify output format (Default: stdout).
+                            Values: stdout, txt, csv, db.
+--list-blocks [arch]       List all available blocks for analysis on specified GPU <arch> (Default: current GPU arch).
+                            Values: gfx908, gfx90a, gfx940, gfx941, gfx942, gfx950, gfx1151
 ```

projects/rocprofiler-compute/.ai/rules/cpp/callback-boundaries.md

@@ -0,0 +1,127 @@
+# C++ Callback Boundaries
+
+Every C++ library we ship runs inside somebody else's callback. The
+rocprofiler-sdk tool callbacks call us. The torch dispatcher calls us through a
+`RecordFunction` observer. Python calls our `extern "C"` functions through
+`ctypes`.
+
+We do not own the thread, we do not own the stack, and we do not own the
+lifetime. That shapes three things at once, which is why they are in one file.
+
+## What thread are we on
+
+Assume any callback can run on a thread we have never seen, at any time,
+concurrently with itself on another thread. Nothing about our own call order
+holds.
+
+On the dispatch path, meaning the rocprofiler-sdk dispatch and record callbacks
+and the torch observer:
+
+- Do not write to files or the console. When you catch an error, log one line
+  to `stderr`.
+- Read-only rocprofiler-sdk queries are fine, as in `record_callback`. Do not
+  create, start, or stop a context from inside a callback.
+- Hold a lock only for a short span. On the steady-state path,
+  `SdkCallbacksImpl::dispatch_callback` takes a `shared_lock` and returns when
+  the agent's profile is already cached.
+- Allocate up front where it is practical, and `reserve()` a container that
+  grows per record.
+- Build expensive state lazily, once, on first use, and cache it. The one-time
+  fill may hold the write lock across that build. Later dispatches must not.
+
+The tools for shared state on that path, in order of preference:
+
+1. `std::atomic` for counters. Use `memory_order_relaxed` when the value is only
+   ever read as a statistic. `Stats` in
+   `src/lib/torch_trace_collector/stats.h` is used this way.
+2. `thread_local` for per-thread state, so there is no sharing to coordinate.
+   `thread_state()` in `src/lib/torch_trace_collector/torch_trace_collector.cpp`
+   holds the marker stack this way.
+3. `synchronized_t<T>` from `src/lib/utils/synchronized/synchronized.hpp` when
+   state really is shared. Shard the data if contention is real.
+   `SnapshotStore` shards its map for that reason.
+
+## What must not cross the boundary
+
+An exception must never escape into rocprofiler-sdk, torch, or the Python
+interpreter. Unwinding through a foreign frame is undefined behaviour, and in
+practice it terminates the profiled application, which is the one thing a
+profiler must not do.
+
+So every function that the host can call directly is a firewall:
+
+- Wrap the body in `try` and catch `...` at the outermost level.
+- Convert the failure into whatever the host expects. A status code for
+  rocprofiler-sdk, a null or a no-op for a torch observer, a status code for
+  an `extern "C"` function that Python calls.
+- Report it. Bump an error counter and write one line to `stderr`. Do not throw,
+  do not allocate a message on the hot path, and do not retry.
+- Leave our own state consistent on the way out. `handle_start_error` in
+  `src/lib/torch_trace_collector/torch_trace_collector.cpp` unwinds the marker
+  stack before it returns, and its cleanup is itself inside a `try`, because
+  cleanup running during error handling must not throw either.
+
+Recovery code that can itself fail gets its own `catch (...)` with an empty
+body. That is the one place where swallowing an exception silently is correct.
+
+## Process-lifetime state
+
+We need state that outlives every object we own. The reason is the callback API,
+not how the library gets loaded: the host calls us, and there is no object of
+ours that owns the state and no parameter we can thread it through.
+
+`LD_PRELOAD` is the wrong test for this. Python loads `torch_trace_collector`
+through `ctypes`, and it still needs a process-wide `ProcessState`.
+
+### Allowed
+
+Use this only when a host callback gives us nowhere else to keep the state.
+
+It must take one of two forms. Which one depends on a single question: **does
+the host call us after our static destructors would have run?**
+
+| | Use | Because |
+|---|---|---|
+| No, or you are not sure | Function-local static behind an accessor | Lazy, thread-safe, and the object cannot be used before it exists |
+| Yes | Never-destroyed heap object behind a namespace-scope reference | A destructor at all is the bug, so there must not be one |
+
+The first is the default. Reach for the second only when you can name the host
+teardown path that forces it, and say so in the comment.
+
+**A function-local static behind an accessor.**
+
+```cpp
+ProcessState& process_state() {
+    static ProcessState state;
+    return state;
+}
+```
+
+Initialization is lazy and thread-safe, and the object cannot be used before it
+exists.
+
+**A never-destroyed heap object behind a namespace-scope reference.** Use this
+only when the host tears down after our static destructors would have run, so a
+destructor at all is the bug. Allocate it with `new` and never delete it. That
+is the exception to the raw-allocation ban in [`core.md`](core.md).
+`rocprofiler_compute_tool.cpp` does this because rocprofiler-sdk calls
+`tool_fini` from its own `_dl_fini`, and the comment above the declarations
+says so.
+
+Either way, never a namespace-scope object whose destructor runs at shutdown.
+
+### Required in both cases
+
+- It stays reachable from tests. Either the accessor hands the object out by
+  reference so consumers can be given a different one, or there is an explicit
+  reset and injection seam. `test_knobs::set_sdk_wrapper` and
+  `test_knobs::reset_cfg` in `rocprofiler_compute_tool.cpp` are that seam.
+- A comment says which host API forces it. Name the API and the callback.
+
+### Banned everywhere else
+
+Process-lifetime state is not a shortcut for passing state between our own
+functions. If the call chain is ours end to end, pass a parameter.
+
+[`testability.md`](testability.md) adds only one thing on top of this: whatever
+form the state takes, the accessor has to stay injectable.

projects/rocprofiler-compute/.ai/rules/cpp/cmake.md

@@ -0,0 +1,105 @@
+# CMake
+
+Modern, target-based CMake. The tree already works this way, so the main job is
+not to regress it.
+
+## Principles
+
+1. Think in targets, not variables.
+2. Say what you want, not how to build it.
+3. Do not pollute the global scope. Settings belong on a target.
+4. Use generator expressions for anything that depends on the configuration.
+5. Use `PUBLIC`, `PRIVATE`, and `INTERFACE` deliberately.
+
+## Target commands, not global ones
+
+```cmake
+# Do not
+include_directories(${PROJECT_SOURCE_DIR}/include)
+add_definitions(-DMY_DEFINE)
+link_directories(${SOME_LIB_DIR})
+
+# Do
+target_include_directories(mylib PUBLIC include/)
+target_compile_definitions(mylib PRIVATE MY_DEFINE)
+target_link_libraries(mylib PUBLIC somelib)
+```
+
+## Visibility
+
+- `PRIVATE`: only this target needs it.
+- `INTERFACE`: only consumers need it.
+- `PUBLIC`: both.
+
+Get this right. A dependency that appears in a public header is `PUBLIC`.
+An implementation detail is `PRIVATE`. `src/lib/rocprofiler_compute_tool/CMakeLists.txt`
+links `compression` as `PRIVATE` for that reason, and says so in a comment.
+
+## List sources explicitly
+
+Never `file(GLOB)` for sources. A glob does not invalidate the build when a file
+appears, so the build silently goes stale. List headers alongside sources so
+they show up in IDEs.
+
+## Language standard
+
+Set C++17 once for the tree, as `src/lib/CMakeLists.txt` does. The only raise
+is `test-torch-trace-collector`, which sets `target_compile_features` to
+`cxx_std_20` so that test can include libtorch headers. Do not use C++20
+features in our sources, including that test. Do not put `-std=c++XX` in
+`target_compile_options`.
+
+Skip that test when PyTorch is absent. If the install is present but incomplete,
+fail configure with a clear message instead of dropping the test or failing at
+link time.
+
+## Compile options
+
+```cmake
+target_compile_options(mylib PRIVATE
+    $<$<CXX_COMPILER_ID:GNU,Clang>:-Wall>
+    $<$<CXX_COMPILER_ID:GNU,Clang>:-Wextra>
+)
+```
+
+## Dependencies
+
+Prefer `find_package` with imported targets:
+
+```cmake
+find_package(Boost 1.70 REQUIRED COMPONENTS filesystem)
+target_link_libraries(myapp PRIVATE Boost::filesystem)
+```
+
+Vendored dependencies live under `src/lib/external/` and come in through
+`add_subdirectory`.
+
+## Header-only libraries
+
+```cmake
+add_library(myheaderlib INTERFACE)
+target_include_directories(myheaderlib INTERFACE include/)
+```
+
+`gsl_assert` and `synchronized` under `src/lib/utils/` are the examples here.
+
+## Tests
+
+Test targets go in a `tests/` subdirectory, added conditionally:
+
+```cmake
+if(ENABLE_TESTS)
+    add_subdirectory(tests)
+endif()
+```
+
+Register them with `add_test` so `ctest` finds them.
+
+## Checklist
+
+- [ ] No `include_directories`, `add_definitions`, or `link_directories`
+- [ ] Every `target_link_libraries` and `target_include_directories` has a
+      visibility keyword, and the choice is correct
+- [ ] Sources listed explicitly, no `file(GLOB)`
+- [ ] Configuration-dependent flags use generator expressions
+- [ ] Tests behind `ENABLE_TESTS`

projects/rocprofiler-compute/.ai/rules/cpp/comments-and-docs.md

@@ -0,0 +1,119 @@
+# C++ Comments and Documentation
+
+Two different things:
+
+| Kind | Purpose | Form | Lives |
+|---|---|---|---|
+| Documentation | Tells a caller how to use an API | Doxygen block, `/** @param @return @throws */` | Above a public API |
+| Inline comment | Explains one non-obvious line | `// short why` | Beside the line |
+
+Code that needs neither is the goal. Reach for either only when the code does
+not answer a question a competent reader will have.
+
+## When to write a Doxygen block
+
+Add one when any of these is true and the answer is not already in the function
+name and signature:
+
+- The function is part of a public API consumed by another module.
+- Units, ownership, or threading are not obvious. "nanoseconds, monotonic",
+  "caller takes ownership", "safe to call from any thread".
+- The function can fail in a way the return type does not advertise.
+- There is a precondition the type system does not express, such as "input must
+  be sorted".
+
+Tags: `@param`, `@return`, `@throws`, `@note`, `@warning`, `@see`,
+`@deprecated`, `@code` / `@endcode`.
+
+```cpp
+/**
+ * Sends data over the connection.
+ * @param data Pointer to the buffer. Not owned.
+ * @param size Number of bytes to send.
+ * @return Number of bytes actually sent.
+ * @throws ConnectionError If the peer has closed.
+ */
+[[nodiscard]] size_t send(const void* data, size_t size);
+```
+
+## When to write an inline comment
+
+Only when all three hold:
+
+1. The reader cannot get the intent from the code.
+2. It stays true when the surrounding code is refactored.
+3. It is not already in the commit message, the PR description, the tracker, or
+   another file.
+
+If one of those fails, drop the comment.
+
+A comment describes the code as it stands. It is not a place to record what you
+tried first, what you rejected, or how the code used to look. That belongs in
+the commit message.
+
+```cpp
+// Good. Each of these carries something the code does not say.
+i++;                       // skip header row
+timeout *= 2;              // exponential backoff
+result |= 0x80;            // protocol sets the MSB on the negative flag
+{                          // caller holds m_mutex
+    state.value = compute(...);
+}
+// workaround for FOO-1234; remove once bar.so >= 2.5 ships
+auto handle = legacy_open(path, /*safe=*/true);
+```
+
+```cpp
+// Bad. Each one restates the code. Delete.
+i++;                        // increment i
+int sum = a + b;            // add a and b
+std::vector<int> numbers;   // vector of numbers
+m_count = 0;                // initialize count to zero
+```
+
+## Things that need no comment at all
+
+- Trivial getters and setters.
+- Self-describing predicates: `is_empty()`, `has_value()`.
+- Language idioms: `std::move(x)`, `std::make_unique<T>()`.
+- Operators with their conventional meaning.
+- Constructors and destructors that do what RAII implies.
+- A single-use internal helper whose name already says what it does.
+- Test bodies. `TEST_F` already names the case.
+
+```cpp
+// Noise
+/**
+ * Returns the size.
+ * @return The size.
+ */
+size_t size() const { return m_size; }
+
+// Fine
+size_t size() const { return m_size; }
+```
+
+## Long comment blocks
+
+Multi-line banners at the top of a file, above a test case, or between helpers
+usually retell something the diff, the test name, or the commit message already
+says. Delete by default:
+
+- "Background" or "Test strategy" preambles in test files.
+- Banners separating helpers whose names already say which case they cover.
+- Comments citing `file:line` somewhere else. Line numbers rot.
+- "This used to be broken because X" above the fix.
+- Decorative dividers like `// === Helpers ===`.
+
+Keep:
+
+- Hidden invariants. "caller holds m_mutex", "must be called once at startup".
+- A workaround with a ticket reference and the condition for removing it.
+- Unit, ownership, or threading notes the type system does not enforce. The
+  comment above `EnvironCache` in
+  `src/lib/rocprofiler_compute_tool/environ_cache.h` is one: it says why the
+  class bypasses `getenv()`, which nothing in the code shows.
+- Doxygen on public APIs.
+
+If removing a comment would not confuse a competent reader a year from now, it
+is noise.