[Compiler] Addressing new compiler warnings#3640
Conversation
Clang enables new lifetime warnings in production and we see build errors due to this with the staging compiler. The attributes added in this PR are suggested by the compiler. However, I'm not very familiar with the code base, so the changes may be incorrect.
|
I came across a couple of more instances of this issue. Will update the PR shortly. |
There was a problem hiding this comment.
Pull request overview
This PR addresses new compiler warnings from Clang's lifetime analysis by adding [[clang::lifetimebound]] attributes to function parameters. The changes prevent potential dangling reference issues by annotating parameters whose lifetimes are bound to the returned reference.
Changes:
- Added
[[clang::lifetimebound]]attributes to function parameters that return references tied to input parameter lifetimes - Applied attributes to stream operator overloads and utility functions across multiple header files
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| include/ck/utility/tuple.hpp | Added lifetimebound attribute to tuple element accessor function |
| include/ck/utility/scheduler_enum.hpp | Added lifetimebound attribute to stream operator for LoopScheduler |
| include/ck/utility/pipeline_enum.hpp | Added lifetimebound attribute to stream operator for PipelineVersion |
| include/ck/utility/amd_wave_read_first_lane.hpp | Added lifetimebound attribute to copy_n function's output iterator |
| include/ck/tensor_operation/gpu/device/tensor_layout.hpp | Added lifetimebound attribute to stream operator for Layout |
| include/ck/library/utility/host_tensor.hpp | Added lifetimebound attribute to LogRange function's stream parameter |
| include/ck/host_utility/io.hpp | Added lifetimebound attribute to stream operator for TensorDescriptor |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| template <typename InputIterator, typename Size, typename OutputIterator> | ||
| __device__ static OutputIterator copy_n(InputIterator from, Size size, OutputIterator to) | ||
| __device__ static OutputIterator | ||
| copy_n(InputIterator from, Size size, [[clang::lifetimebound]] OutputIterator to) |
There was a problem hiding this comment.
The [[clang::lifetimebound]] attribute on the OutputIterator parameter is incorrect. This function returns the iterator by value after advancing it, not a reference bound to the input parameter's lifetime. The lifetimebound attribute should only be applied when the return value's lifetime is directly tied to the parameter (e.g., returning a reference to data owned by the parameter). Remove this attribute as it doesn't apply to this pattern.
| copy_n(InputIterator from, Size size, [[clang::lifetimebound]] OutputIterator to) | |
| copy_n(InputIterator from, Size size, OutputIterator to) |
The number of instances was large, so I decided to use file-level scope to disable the warning via pragma clang diagnostic ignored. It also showed this warning coming from the gtest dependency. For that, I did add the respective command line flag to the CMake variables. I don't know if this is acceptable or not.
For a build on gfx90a.
## Motivation The staging compiler enables lifetime-safety warnings and we already worked around a few of them. This works around a few more instances that came up recently on gfx950 builds. The initial PR that resolved most issues: ROCm/composable_kernel#3640 ## Technical Details This follows the pattern to locally ignore the newly added lifetime-safety warnings that were moved from experimental to production in upstream LLVM. As a result, CK turned them on and treats them as errors, which prevents the staging compiler from building CK. ## Test Plan ## Test Result ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. Co-authored-by: Illia Silin <98187287+illsilin@users.noreply.github.com>
[CK] Work around staging compiler lifetime warning ## Motivation The staging compiler enables lifetime-safety warnings and we already worked around a few of them. This works around a few more instances that came up recently on gfx950 builds. The initial PR that resolved most issues: #3640 ## Technical Details This follows the pattern to locally ignore the newly added lifetime-safety warnings that were moved from experimental to production in upstream LLVM. As a result, CK turned them on and treats them as errors, which prevents the staging compiler from building CK. ## Test Plan ## Test Result ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. Co-authored-by: Illia Silin <98187287+illsilin@users.noreply.github.com>
## Motivation The staging compiler enables lifetime-safety warnings and we already worked around a few of them. This works around a few more instances that came up recently on gfx950 builds. The initial PR that resolved most issues: ROCm/composable_kernel#3640 ## Technical Details This follows the pattern to locally ignore the newly added lifetime-safety warnings that were moved from experimental to production in upstream LLVM. As a result, CK turned them on and treats them as errors, which prevents the staging compiler from building CK. ## Test Plan ## Test Result ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. Co-authored-by: Illia Silin <98187287+illsilin@users.noreply.github.com>
## Motivation The staging compiler enables lifetime-safety warnings and we already worked around a few of them. This works around a few more instances that came up recently on gfx950 builds. The initial PR that resolved most issues: ROCm/composable_kernel#3640 ## Technical Details This follows the pattern to locally ignore the newly added lifetime-safety warnings that were moved from experimental to production in upstream LLVM. As a result, CK turned them on and treats them as errors, which prevents the staging compiler from building CK. ## Test Plan ## Test Result ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. Co-authored-by: Illia Silin <98187287+illsilin@users.noreply.github.com>
Proposed changes
Clang enables new lifetime warnings in production and we see build errors due to this with the staging compiler.
The attributes added in this PR are suggested by the compiler. However, I'm not very familiar with the code base, so the changes may be incorrect.
Checklist
Please put an
xinto the boxes that apply. You can also fill these out after creating the PR. If you're not sure, please don't hesitate to ask.clang-formaton all changed filesDiscussion