Add pytorch inspired DeviceTransform benchmark#9764
Conversation
a844355 to
4a9f9fe
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new NVBench CUDA benchmark with a replicated ChangesChained elementwise transform benchmarks
Suggested reviewers: Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cub/benchmarks/bench/transform/applications/P1/pytorch.cu (1)
258-265: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winsuggestion: these comparators take non-const references, so they can't bind const operands or temporaries — the very thing
std::min/std::max(const-ref params) require, contradicting the comment. In this filestd::min/std::maxare all called onopmath_t(float), so as written these are effectively unused; if ever hit on a constBFloat16they only compile by falling back to the implicit float conversion. Useconst&.-inline __host__ __device__ bool operator>(BFloat16& lhs, BFloat16& rhs) +inline __host__ __device__ bool operator>(const BFloat16& lhs, const BFloat16& rhs) { return float(lhs) > float(rhs); } -inline __host__ __device__ bool operator<(BFloat16& lhs, BFloat16& rhs) +inline __host__ __device__ bool operator<(const BFloat16& lhs, const BFloat16& rhs) { return float(lhs) < float(rhs); }As per coding guidelines: "All variables that are not modified must be declared
const".Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a9c3b764-29b6-4949-87f1-04bc6015f851
📒 Files selected for processing (2)
cub/benchmarks/CMakeLists.txtcub/benchmarks/bench/transform/applications/P1/pytorch.cu
This comment has been minimized.
This comment has been minimized.
7159511 to
63f97a2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cub/benchmarks/bench/transform/applications/P1/bfloat16.h (1)
6-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winsuggestion: This header relies on transitive includes for several symbols:
uint16_t/UINT16_C/UINT32_C(<cstdint>),__nv_bfloat16/__float2bfloat16/__bfloat16_as_ushort/__bfloat162float(<cuda_bf16.h>), andNV_IF_TARGET/NV_IS_DEVICE(<nv/target>). Include them directly.`#include` <cmath> +#include <cstdint> `#include` <cstring> +#include <cuda_bf16.h> +#include <nv/target> + `#include` <nvbench/type_strings.cuh>As per coding guidelines: "Include all headers needed by the symbols being used; do not rely on transitive includes."
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a045ad98-301f-4eef-8321-e761f9ec9a95
📒 Files selected for processing (3)
cub/benchmarks/CMakeLists.txtcub/benchmarks/bench/transform/applications/P1/bfloat16.hcub/benchmarks/bench/transform/applications/P1/pytorch.cu
🚧 Files skipped from review as they are similar to previous changes (1)
- cub/benchmarks/CMakeLists.txt
This comment has been minimized.
This comment has been minimized.
😬 CI Workflow Results🟥 Finished in 1h 06m: Pass: 58%/242 | Total: 1d 08h | Max: 1h 06m | Hits: 99%/87559See results here. |
This benchmark is donated by @MatthiasKohl.
Q: How much should be simplify it by adopting primitives from CCCL?
Running locally:
Fixes: https://github.com/NVIDIA-dev/cccl_private/issues/639