Skip to content

Port publication and performance utility fixes - #2449

Open
umangyadav wants to merge 2 commits into
developfrom
users/umayadav/port-publication-fixes
Open

Port publication and performance utility fixes#2449
umangyadav wants to merge 2 commits into
developfrom
users/umayadav/port-publication-fixes

Conversation

@umangyadav

Copy link
Copy Markdown
Member

Summary

  • remove internal-only links and personal contact details from public-facing source, tests, and metadata while preserving plain-text ticket provenance
  • add publication hygiene to the PR review checklist and use the public ROCm support contact for package/container metadata
  • use LLVM's IEEE half conversion, correct F16/BF16 host constants, and preserve missing report data when identifying constant columns

Test plan

  • Build benchmark-driver-utils and rocmlir-tuning-driver in the shared-library configuration
  • Configure BUILD_FAT_LIBROCKCOMPILER=ON, build the static benchmark library and tuning driver, and smoke-test rocmlir-tuning-driver --help
  • Verify F16/BF16 host-constant encodings plus Inf/NaN handling
  • Run flake8, YAPF, TOML parsing, pandas missing-data regression checks, and clang-format
  • Verify generated CPack maintainer metadata and runtime linkage
  • Full local premerge checker: clang-format passed; clang-tidy could not locate mlir/Dialect/Affine/Analysis/LoopAnalysis.h because the checker did not find its expected compile database

Back-ports the functional and publication-hygiene changes identified while reviewing ROCm/rocmlirTriton#420. Buildbot scripts and external/ are intentionally unchanged.

Made with Cursor

umangyadav and others added 2 commits August 13, 2026 14:40
Use LLVM's IEEE half conversion, correct F16/BF16 host constants, and preserve missing report data when identifying constant columns.

Co-authored-by: Cursor <cursoragent@cursor.com>
Retain ticket provenance without internal links, replace personal contacts with public support channels, and add publication hygiene to the review checklist.

Co-authored-by: Cursor <cursoragent@cursor.com>
@umangyadav
umangyadav requested a review from causten as a code owner August 13, 2026 15:32
@umangyadav
umangyadav requested a lite review from Copilot August 13, 2026 15:32
@umangyadav umangyadav self-assigned this Aug 13, 2026
@umangyadav umangyadav added the claude-review Trigger automated PR review by claude[bot]; auto-removed after the run. label Aug 13, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves publication hygiene across documentation/tests/metadata and fixes correctness in performance utilities by switching to LLVM’s IEEE half conversion, correcting F16/BF16 constant encodings, and preserving missing-data columns in reports.

Changes:

  • Remove internal-only URLs/personal contacts from public-facing sources while preserving plain-text ticket provenance.
  • Fix performance utilities: use llvm::APFloat for IEEE half conversion, correct host-constant encodings for F16/BF16, and avoid dropping all-missing report columns.
  • Update package/container maintainer metadata and extend the PR review checklist with publication-hygiene checks.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
README.md Replaces direct maintainer contact with issue link.
mlir/utils/performance/reportUtils.py Reworks constant-column detection to preserve all-missing columns.
mlir/utils/performance/common/CMakeLists.txt Adds LLVM dependency for updated half conversion implementation.
mlir/utils/performance/common/benchmarkUtils.cpp Switches FP16 conversion to llvm::APFloat and fixes F16/BF16 host constants.
mlir/utils/jenkins/Dockerfile Replaces deprecated MAINTAINER with a public maintainer label.
mlir/tools/rocmlir-lib/CMakeLists.txt Updates package maintainer metadata to ROCm Dev Support.
mlir/test/fusion/pr-e2e/gemm-layouts/other/broadcasted-b-e2e.mlir Removes internal URL; keeps issue provenance as plain text.
mlir/test/fusion/pr-e2e/attention/mixr-attention-small-decode.mlir Removes internal URL; keeps ticket provenance as plain text.
mlir/test/fusion/nightly-misc-e2e/issue-940.mlir Removes internal URL; keeps issue provenance as plain text.
mlir/test/fusion/linalg-generic-const-initializer.mlir Replaces internal URL with plain-text issue provenance.
mlir/test/e2e/conv_regression_fwd.toml Removes internal links and normalizes provenance comments.
mlir/test/e2e/conv_regression_fwd_nonNavi3x.toml Removes internal links and normalizes provenance comments.
mlir/test/e2e/conv_regression_fwd_navi3x.toml Removes internal links and normalizes provenance comments.
mlir/test/e2e/conv_regression_bwd.toml Removes internal links and normalizes provenance comments.
mlir/lib/Dialect/Rock/Transforms/GridwiseGemmToBlockwise.cpp Rewrites TODO to remove internal URL while keeping provenance.
mlir/include/mlir/Dialect/Rock/IR/RockAttrDefs.td Removes internal URL from attribute docs; keeps provenance.
docs/PR_REVIEW_CHECKLIST.md Adds explicit checklist item to block internal-only URLs in public sources.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines 4 to +8
add_library(benchmark-driver-utils EXCLUDE_FROM_ALL benchmarkUtils.cpp)
target_link_libraries(benchmark-driver-utils PUBLIC hip::host hip::amdhip64)
target_link_libraries(benchmark-driver-utils
PUBLIC hip::host hip::amdhip64
PRIVATE LLVMSupport
)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That suggestion is incorrect: CMake propagates private dependencies of static libraries as link-only dependencies to final executables. PRIVATE correctly avoids leaking LLVM compile usage requirements to consumers.

@rocmlir-pr-reviewer rocmlir-pr-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVE -- submitted as COMMENT (automated reviews are advisory)  ·  Findings: 0 (0 Critical, 0 Major, 0 Minor)


Scope

Publication-hygiene cleanup plus three small functional fixes, back-ported from a downstream review. Non-functional: replaces internal issue-tracker/Confluence-style hyperlinks with plain-text ticket identifiers across README.md, RockAttrDefs.td, GridwiseGemmToBlockwise.cpp, four mlir/test/e2e/*.toml files and four mlir/test/fusion/*.mlir files; switches package/container maintainer metadata to a public support alias; adds a Critical checklist bullet forbidding internal-only URLs. Functional: float_to_float16 now delegates to llvm::APFloat, makeHostConstant no longer swaps the F16/BF16 encoders, and reportUtils.unique_cols becomes constant_columns with NA-aware semantics.

Findings

No blocking issues found.

Notes

  • The F16/BF16 swap at mlir/utils/performance/common/benchmarkUtils.cpp:413-421 is a genuine bug fix: getPattern (same file, lines 133-148) already used the correct mapping, so only makeHostConstant was inconsistent. Worth calling out to whoever tracks perf history — F16/BF16 fusion constants change value across this commit, so pre/post benchmark numbers for those dtypes are not directly comparable.
  • llvm::APFloat::convert is not [[nodiscard]] in the vendored LLVM (external/llvm-project/llvm/include/llvm/ADT/APFloat.h:1381), so discarding the opStatus will not warn. The new implementation also fixes the old saturate path, which returned 0x7FFF (a NaN) on overflow instead of inf.
  • Re: the existing reviewer comment on mlir/utils/performance/common/CMakeLists.txtPRIVATE LLVMSupport is correct here and does not need to be PUBLIC. For a static library CMake records private dependencies as $<LINK_ONLY:LLVMSupport> in INTERFACE_LINK_LIBRARIES, so rocmlir-tuning-driver and hipblaslt-benchmark-driver still get LLVMSupport on their link lines in the BUILD_FAT_LIBROCKCOMPILER=ON configuration. PRIVATE is also the accurate choice because APFloat.h is included only from the .cpp, not from benchmarkUtils.h. Include paths resolve via include_directories(${LLVM_INCLUDE_DIRS}) at mlir/CMakeLists.txt:26.
  • constant_columns in mlir/utils/performance/reportUtils.py:118-127 also fixes an incidental crash: the old numpy a[0] == a form raised IndexError on an empty frame, whereas nunique returns 0 and drops nothing. I checked perfRegressionReport.py, createPerformanceReports.py and createFusionPerformanceReports.py — none referenced the old unique_cols name, so the rename has no other callers.
  • The .toml hunks only collapse banner comments; I verified the line-count deltas so that no [[suite.test]] entry was dropped.
  • Testing gap (non-blocking): the makeHostConstant correction has no automated regression test. The conversion helpers live in an anonymous namespace in the .cpp, so a unit test would need them exposed first — probably not worth doing in this PR, but the fix is currently only covered by the manual verification described in the PR body.

CI status

No failing or cancelled checks. Python format and lint checks, Python performance script tests and Detect relevant changes pass; C/C++ premerge checks was still in progress at review time and should be confirmed green before merge, since benchmark-driver-utils is EXCLUDE_FROM_ALL and only builds when ROCMLIR_ENABLE_BENCHMARKS or the tuning driver is enabled.

@rocmlir-pr-reviewer rocmlir-pr-reviewer Bot removed the claude-review Trigger automated PR review by claude[bot]; auto-removed after the run. label Aug 13, 2026
@umangyadav
umangyadav requested a lite review from Copilot August 13, 2026 18:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants