adding ze_validator to arginne-lcf/devel by myrepo1 · Pull Request #529 · argonne-lcf/THAPI

myrepo1 · 2026-08-11T20:32:03Z

Refactored the ze_validator:

Split into multiple files
Executable to profile device properties (part of the validator)
Added multiple new detections

TApplencourt · 2026-08-11T20:42:59Z

+6,802-6,031. : ) I hope you didn't rebase correctly as I don't expect this PR to change the extract and ze_api.h

TApplencourt · 2026-08-12T15:15:14Z

+            context['hostname'] = hostname
+            context['api'] = m[1]
+			      #zeDriversInit or zeInit must be the first one to be called before any api calls
+            check_initialization(context,m)


check_init just need m[1], but you added m[1] into context just before... so why are you passing both context and m1..?

TApplencourt · 2026-08-12T15:15:35Z

+              #puts "#{m[1]}"
+              on_exit(m, hostname, context, defi)
+            end
+            #ADDED: this event may have signaled something a deferred command list


What are all those "ADDED"?! What is the point of this word?!

TApplencourt · 2026-08-12T15:16:47Z

+            hostname = e.stream.trace.get_environment_entry_value_by_name('hostname').value
+            context = e.get_common_context_field.value
+            #the event's own fields: input args at _entry, results at _exit
+            defi = e.payload_field.value


why defi? What is the meeting of defi ? why not payload_filed?

Oh found in to the top

defi -- the decoded event payload ("definition").`

Definition... I will vote for payload ...

TApplencourt · 2026-08-12T15:18:35Z

+          #fall through the `if` and are ignored.
+          m = e.name.match(/:(z.*)_(entry|exit)/)
+          if m
+            #hostname is trace-level metadata (recorded once per trace), while


why it's important here to said that hostname is recorde one per trace? What does this add to the code to know that?

TApplencourt · 2026-08-12T15:19:13Z

+            #hostname is trace-level metadata (recorded once per trace), while
+            #vpid/vtid come from LTTng's per-event common context. Merging them
+            #with the API name produces the `context` tuple that identifies
+            #"who is calling what", threaded through the entire validator.


threaded through ? wath does that mean threaded through ? So much fancy english

TApplencourt · 2026-08-12T15:22:11Z

+              print_leak_error(ctx, t, h) #it prints the type as well
+            }
+          }
+          # CHANGED: memory_allocation is now nested by context handle


???
Really? You think comment should refer old commit information? We have git-blame if we want to know how old version of the code is doing...

Please, review everything and ask yourself if the comment make any sense, and that every word of the comment add MEANINGFUL information, not slop who just add noise.

TApplencourt · 2026-08-12T15:29:00Z

@@ -0,0 +1,29 @@
+---
+zeCommandListDestroy:


what is the point of the second argument, if everything is all the time command_list?
Can we add some with event maybe?

TApplencourt · 2026-08-12T15:36:43Z

Oh and yeah, maybe a better name for the PR will help... Pull request into devel is not that supper meaningful

TApplencourt · 2026-08-12T15:40:00Z

+# earlier version of the model and explain WHY the change was necessary. They
+# are kept because the reasoning (e.g. why memory is keyed by context) is not
+# recoverable from the code alone.
+# =============================================================================


... Comment are here to explain context that are not "recoverable from the code"
It doesn't matter AT ALL if we was Added changed removed. Only the current version of the code mattern

TApplencourt · 2026-08-12T15:40:50Z

+  #One of these APIs must be called before any other calls
+  # The Level Zero spec requires the runtime to be initialized before any other
+  # entry point is used. Calling anything else first is undefined behavior, so
+  # StateObject#check_initialization watches for one of these appearing first.
+  INIT_API_NAMES = ['zeInit', 'zeInitDrivers']


No need to rexplain the same thing into 20 difference place.
You explain the except behavior in check_initialization. I think we got it.

TApplencourt · 2026-08-12T15:41:53Z

+      @sub_devices = []
+      @memory_allocations = Hash.new { |h, k| h[k] = {} }
+      @property_fetched = false
+      #This


TApplencourt · 2026-08-12T15:42:44Z

+  # No need to create DeviceMemory class. Just create Memory with the specified type (device,shared,host)
+  # class DeviceMemory < Object
+  #   @typename = 'memory_allocation_device'
+  #   attr_reader :context
+  #   attr_reader :size
+  #   attr_reader :device
+
+  #   def initialize(handle, context, size, device)
+  #     super(handle,context,size,device)
+  #     @context = context
+  #     @size = size
+  #     @device = device
+  #   end
+  # end
+


Please do not commit commented code

TApplencourt · 2026-08-12T15:44:21Z

+    # fill/kernel that still references its address range can be reported as a
+    # use-after-free instead of silently passing (unknown pointer).
+    attr_accessor :freed_by
+    def initialize(handle, context, size, owned_by, memtypestr="shared")


why by default it should be shared?

TApplencourt · 2026-08-12T15:45:16Z

+      # Device allocations must be explicitly made resident before the GPU can
+      # use them; host and shared memory is always accessible, so it starts
+      # resident and stays that way.


What? Are you sure of that? Where does the spec said that? And you have a test who verify this?
AFAIK device allocation will automaticaly be made resident the first time they have been used

It is a little complicated as the driver usually makes things resident and it depends on if the device supports page-faults (https://oneapi-src.github.io/level-zero-spec/level-zero/latest/core/PROG.html#device-residency). if the memory is passed as a kernel argument we don't need to explicitly make it resident but for things like indirect accesses we will.

so maybe not "Device allocations must be explicitly made resident" but "Device allocations must be resident" (but in many cases this happens under the hood)

TApplencourt · 2026-08-12T15:47:40Z

+
+    def signaled?
+      @signaled
+    end


why not just just use signaled? or signaled everywhere then? What does this new keyword add us?

Replay the ze_validator work on top of alcf/devel (ab4bc29) without the accidental reverts of upstream that had accumulated on first-half-merge (notably PR argonne-lcf#484 'H2yaml is now mandatory' and the Level Zero 1.18.2 header update, which commit 4c34a85 had rolled back). Adds: ze_validator.in, ze_validator_zemodel.rb, ze_validator_function_entry_exit_callbacks.rb, ze_validator_entry_exit_helpers.rb, ze_validator_state_object.rb, ze_thread_safety.yaml, ze_deprecated.json, ze_device_property.cpp Build glue: configure.ac generates backends/ze/ze_validator from ze_validator.in Makefile.am installs the validator script, builds ze_device_property with icpx, and ships the validator data files via data_DATA/EXTRA_DIST.

rpereira-dev · 2026-08-12T18:05:45Z

+#     exactly this.
+#
+# TYPICAL USE
+# -----------


@TApplencourt I suggest we keep usage as documented here.
In the future, we want to add a new CLA on validation passes to run. e.g.: iprof --validate mpi,ze [...] -- that automatically sets the appropriate backends (mpi,ze) and run validators after the execution completed

myrepo1 · 2026-08-12T18:07:22Z

@@ -0,0 +1,147 @@
+// ze_device_property.cpp


This program calls zeDeviceGetCommandQueueGroupProperties for each device a host can access, and maps the ordinal index to the compute/copy engines. This program gets executed at validation time by the validator, and the mapping will be saved as a ze_device_property.json file (to avoid subsequent calls to this program at later validation times). The resulting mapping gets used for checking submission of kernels to command lists and command queues that are associated with copy-only engines

with the ordinal PR #526, is this still needed?

myrepo1 · 2026-08-12T18:15:27Z


 bin_SCRIPTS = \
-	tracer_ze.sh
+	tracer_ze.sh \


For compiling the ze_device_property.cpp
Binary gets stored to build/ici/bin

myrepo1 · 2026-08-12T18:19:15Z

@@ -0,0 +1,8 @@
+{


This stores deprecated APIs. The first field inside the bracket is the version number the API got deprecated from (e.g., "1.10"), and the second field denotes recommended alternative (e.g., "zeInitDrivers").

colleeneb · 2026-08-12T18:40:59Z

+#     -> pump_deferred (a signal may have unblocked pending GPU work)
+#   ...and once the trace is exhausted, ze_validator.in calls #check_issues.
+#
+# TWO VOCABULARY NOTES THAT UNLOCK THE REST OF THE CODE


These comments can be simplified likely as well. Good to have overview but maybe a little less.

rpereira-dev · 2026-08-12T18:30:30Z

+    def immediate?
+      return !desc
+    end
+  end


Too many comments everywhere, here is a good example (4 lines of comments for 1 of code)
All these comments may run out-of-sync if edits are made (to Level Zero, or to the validator itself) -- removing them will ease maintenance.

Level Zero semantics is well-defined and the code should be self-contained: the spec is the doc

rpereira-dev · 2026-08-12T18:34:36Z

+# are kept because the reasoning (e.g. why memory is keyed by context) is not
+# recoverable from the code alone.
+# =============================================================================
+module ZEModel


I'll join Thomas: too much comments.
Just replace these 50 lines of comments with 1 line, e.g.:

This file implements Level Zero objects (https://oneapi-src.github.io/level-zero-spec)

rpereira-dev · 2026-08-12T18:39:51Z

+  #one of them turns up a cycle
+  stuck.each { |u| break if dfs.call(u); }
+  report_deadlock_cycle(state, found) if found
+end


I suggest using a graph library instead of reimplementing those standard algorithm.
This: https://github.com/monora/rgl - can detect cycle: https://rubydoc.info/github/monora/rgl/RGL/Graph#acyclic%3F-instance_method

Work for me, If using any dependency, they need to be packaged in spack or in https://github.com/argonne-lcf/THAPI-spack

colleeneb · 2026-08-12T22:17:13Z

+
+#Find the memory allocation containing the ptr
+def find_allocation_containing(allocations, ptr)
+  allocations.each_value.find { |m| m.base && m.base <= ptr && ptr < m.base + m.size }


If I interpret right, it looks like find is O(N) (https://www.rubydoc.info/stdlib/core/Enumerable:find) so this will blow up for traces with a lot of memory allocations. Either an IntervalTree or a sorted map data type should help so this could go to O(N). This can be added in the future but I want to get it in the review so it's not forgotten (O(N) blows up fast and has bitten us before)

TApplencourt reviewed Aug 12, 2026

View reviewed changes

myrepo1 force-pushed the first-half-merge branch from 3bf303a to 7aafbc2 Compare August 12, 2026 17:50

rpereira-dev reviewed Aug 12, 2026

View reviewed changes

myrepo1 commented Aug 12, 2026

View reviewed changes

myrepo1 changed the title ~~Pull request into devel~~ adding ze_validator to arginne-lcf/devel Aug 12, 2026

colleeneb reviewed Aug 12, 2026

View reviewed changes

rpereira-dev suggested changes Aug 12, 2026

View reviewed changes

WIP: cleaning up comments

7a42cd0

colleeneb reviewed Aug 12, 2026

View reviewed changes

		@@ -0,0 +1,29 @@
		---
		zeCommandListDestroy:

Uh oh!

Conversation

myrepo1 commented Aug 11, 2026

Uh oh!

TApplencourt commented Aug 11, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TApplencourt Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TApplencourt Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

defi -- the decoded event payload ("definition").`

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TApplencourt commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TApplencourt Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rpereira-dev Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

TApplencourt Aug 12, 2026 •

edited

Loading

TApplencourt Aug 12, 2026 •

edited

Loading

`defi` -- the decoded event payload ("definition").`

TApplencourt commented Aug 12, 2026 •

edited

Loading

TApplencourt Aug 12, 2026 •

edited

Loading

rpereira-dev Aug 12, 2026 •

edited

Loading