Forward PreRun/PostRun to all operands in composing operators#1200
Conversation
Several operators that accept operator-valued operands forwarded PreRun()
and PostRun() to only a subset of their operands. The dropped operands were
still read lazily in the operator's kernel.
When a dropped operand is a transform-style operator that allocates and fills
temporary storage during PreRun, that temporary was never allocated or computed,
so the consuming operator read uninitialized memory.
Operators fixed (each now forwards both PreRun and PostRun to the
previously-dropped operand, guarded by is_matx_op<>()):
- polyval : coefficient operand
- interp1 : sample points, sample values, and query points (the default
LINEAR method's PreRun was otherwise a no-op)
- remap : index operand
- shift : shift-amount operand
- sar_bp : voxel_locations and range_to_mcp
Each fix adds a self-contained regression test that composes the operator with
a new prerun_tester operator that validates PreRun forwarding of the parent
operator.
Signed-off-by: Thomas Benson <tbenson@nvidia.com>
Greptile SummarySeveral composing operators (
Confidence Score: 5/5Safe to merge. All five operators now correctly forward PreRun/PostRun to every operator-valued operand, eliminating the uninitialized-memory reads described in the PR. The capability bookkeeping is updated consistently. The fix is surgical and follows the well-established forwarding pattern already used for the previously forwarded operands in each operator. The new PreRunTesterOp infrastructure cleanly validates the forwarding contract without coupling to operator internals. The sar_bp two-level PreRun design (InnerPreRun → PreRun) is preserved correctly. No regressions are expected. The interp1 regression test only exercises the CUDA executor path — a pre-existing static_assert in interp.h triggers for non-CUDA executors even when the LINEAR method is selected, so the LINEAR fix on host executors is not directly covered by the new test. Important Files Changed
Sequence DiagramsequenceDiagram
participant R as run()
participant C as ComposingOp
participant A as Operand A (previously forwarded)
participant B as Operand B (newly forwarded)
R->>C: PreRun(shape, ex)
C->>A: "is_matx_op<A>? PreRun(shape, ex)"
Note over A: allocates/fills temp storage
C->>B: "is_matx_op<B>? PreRun(shape, ex) NEW"
Note over B: allocates/fills temp storage (was skipped before fix)
C->>C: own allocations
R->>C: Exec(shape, ex)
C->>A: operator()(indices)
C->>B: operator()(indices) reads valid temp storage now
R->>C: PostRun(shape, ex)
C->>A: "is_matx_op<A>? PostRun(shape, ex)"
C->>B: "is_matx_op<B>? PostRun(shape, ex) NEW"
C->>C: matxFree own allocations
Reviews (1): Last reviewed commit: "Forward PreRun/PostRun to all operands i..." | Re-trigger Greptile |
|
/build |
|
Coverage is 93.581% — tbenson/fix-operators-missing-prerun-forwarding into main. No base build found for |
3 participants
Several operators that accept operator-valued operands forwarded PreRun() and PostRun() to only a subset of their operands. The dropped operands were still read lazily in the operator's kernel.
When a dropped operand is a transform-style operator that allocates and fills temporary storage during PreRun, that temporary was never allocated or computed, so the consuming operator read uninitialized memory.
Operators fixed (each now forwards both PreRun and PostRun to the previously-dropped operand, guarded by is_matx_op<>()):
Each fix adds a self-contained regression test that composes the operator with a new prerun_tester operator that validates PreRun forwarding of the parent operator.