Skip to content

[SYCLLowerIR] Remove !amdgcn.annotations metadata #14713

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 29, 2024

Conversation

frasercrmck
Copy link
Contributor

@frasercrmck frasercrmck commented Jul 23, 2024

The !amdgcn.annotations metadata was a SYCL-specific addition. The concept of annotations for AMDGPU makes it appear as if it's a mirror of NVVM annotations, when in fact it's just a kernel tagging mechanism. It is not a feature supported by AMD's drivers. We don't need to rely on this, as the functions' calling conventions identify kernels. We also rely on the "sycl-device" module flag to restrict the passes to SYCL code.

This patch re-uses the existing TargetHelpers namespace to hide the target-specific logic behind a new class: the KernelCache. This provides a way of maintaining a cache of kernels, with optional annotation metadata (it could be expanded in the future with more types of payload). It also provides abstracted ways of handling certain RAUW operations on kernels, though currently only a minimum required to support the two existing patterns. The aim of this is to hide all concept of "annotations" from the passes, and make it an implementation detail of the KernelCache.

During this work, it was noticed that our handling of annotations was incomplete. NVVM annotations are not required to only only have 3 operands, as the official documentation shows. It's actually a list of pairs, any one of which may declare the function a kernel. Thus we may have missed valid kernels. Tests have been added to check for this.

The GlobalOffset pass was also treating "unsupported" architectures as AMDGPU architectures, so that has been tightened up and the tests have been updated to ensure they actually register as AMD modules.

LIT tests have been cleaned up somewhat, to remove unnecessary features like comments and function linkage types.

Several LIT tests have been converted to use
the update_test_checks.py or update_llc_test_checks.py scripts, where appropriate. These tools cannot currently emit checks for named metadata nor certain assembly features, so some tests must remain as they are.

The `!amdgcn.annotations` metadata was a SYCL-specific addition. The
concept of annotations for AMDGPU makes it appear as if it's a mirror of
NVVM annotations, when in fact it's just a kernel tagging mechanism. It
is not a feature supported by AMD's drivers. We don't need to rely on
this, as the functions' calling conventions identify kernels. We also
rely on the "sycl-device" module flag to restrict the passes to SYCL
code.

This patch re-uses the existing `TargetHelpers` namespace to hide the
target-specific logic behind a new class: the `KernelCache`. This
provides a way of maintaining a cache of kernels, with optional
annotation metadata (it could be expanded in the future with more types
of payload). It also provides abstracted ways of handling certain RAUW
operations on kernels, though currently only a minimum required to
support the two existing patterns. The aim of this is to hide all
concept of "annotations" from the passes, and make it an implementation
detail of the `KernelCache`.

During this work, it was noticed that our handling of annotations was
incomplete. NVVM annotations are not required to only only have 3
operands, as the official documentation shows. It's actually a list of
pairs, any one of which may declare the function a kernel. Thus we may
have missed valid kernels. Tests have been added to check for this.

The `GlobalOffset` pass was also treating "unsupported" architectures as
AMDGPU architectures, so that has been tightened up and the tests have
been updated to ensure they actually register as AMD modules.

LIT tests have been cleaned up somewhat, to remove unnecessary features
like comments and function linkage types.

Several LIT tests have been converted to use
the update_test_checks.py or update_llc_test_checks.py scripts, where
appropriate. These tools cannot currently emit checks for named metadata
nor certain assembly features, so some tests must remain as they are.
@frasercrmck frasercrmck requested review from a team as code owners July 23, 2024 12:31
Copy link
Contributor

@sommerlukas sommerlukas left a comment

Choose a reason for hiding this comment

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

Only reviewed kernel fusion changes, those LGTM.

Copy link
Contributor

@premanandrao premanandrao left a comment

Choose a reason for hiding this comment

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

FE changes look okay to me.

@frasercrmck
Copy link
Contributor Author

Ping @intel/dpcpp-tools-reviewers, thanks. I'd like to merge this before #14634 which I need to merge before I can continue with #14518 which is high priority.

@maksimsab
Copy link
Contributor

Sorry for the waiting. I will take a look today.

Copy link
Contributor

@maksimsab maksimsab left a comment

Choose a reason for hiding this comment

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

LGTM

@bader bader merged commit dc37699 into intel:sycl Jul 29, 2024
16 checks passed
@sarnex
Copy link
Contributor

sarnex commented Jul 29, 2024

@frasercrmck We are seeing postcommit failures:

 /__w/llvm/llvm/src/sycl-fusion/passes/kernel-fusion/SYCLSpecConstMaterializer.cpp:301:28: error: no member named 'getArchType' in namespace 'llvm::TargetHelpers'
  301 |   auto AT = TargetHelpers::getArchType(*Mod);
      |             ~~~~~~~~~~~~~~~^
/__w/llvm/llvm/src/sycl-fusion/passes/kernel-fusion/SYCLSpecConstMaterializer.cpp:302:22: error: no member named 'ArchType' in namespace 'llvm::TargetHelpers'
  302 |   if (TargetHelpers::ArchType::Cuda != AT &&
      |       ~~~~~~~~~~~~~~~^
/__w/llvm/llvm/src/sycl-fusion/passes/kernel-fusion/SYCLSpecConstMaterializer.cpp:303:22: error: no member named 'ArchType' in namespace 'llvm::TargetHelpers'
  303 |       TargetHelpers::ArchType::AMDHSA != AT) {
      |       ~~~~~~~~~~~~~~~^
3 errors generated.
ninja: build stopped: subcommand failed.

https://github.com/intel/llvm/actions/runs/10148224615/job/28060449427

Can you please take a look?

@frasercrmck
Copy link
Contributor Author

@frasercrmck We are seeing postcommit failures:

 /__w/llvm/llvm/src/sycl-fusion/passes/kernel-fusion/SYCLSpecConstMaterializer.cpp:301:28: error: no member named 'getArchType' in namespace 'llvm::TargetHelpers'
  301 |   auto AT = TargetHelpers::getArchType(*Mod);
      |             ~~~~~~~~~~~~~~~^
/__w/llvm/llvm/src/sycl-fusion/passes/kernel-fusion/SYCLSpecConstMaterializer.cpp:302:22: error: no member named 'ArchType' in namespace 'llvm::TargetHelpers'
  302 |   if (TargetHelpers::ArchType::Cuda != AT &&
      |       ~~~~~~~~~~~~~~~^
/__w/llvm/llvm/src/sycl-fusion/passes/kernel-fusion/SYCLSpecConstMaterializer.cpp:303:22: error: no member named 'ArchType' in namespace 'llvm::TargetHelpers'
  303 |       TargetHelpers::ArchType::AMDHSA != AT) {
      |       ~~~~~~~~~~~~~~~^
3 errors generated.
ninja: build stopped: subcommand failed.

https://github.com/intel/llvm/actions/runs/10148224615/job/28060449427

Can you please take a look?

Looks like #14280 was merged in first and caused the issues. Ideally CI would have been re-run before merging this.

@frasercrmck
Copy link
Contributor Author

See #14833

@frasercrmck frasercrmck deleted the amdgpu-cc-kernels branch July 29, 2024 18:59
AlexeySachkov pushed a commit to AlexeySachkov/llvm that referenced this pull request Nov 26, 2024
The `!amdgcn.annotations` metadata was a SYCL-specific addition. The
concept of annotations for AMDGPU makes it appear as if it's a mirror of
NVVM annotations, when in fact it's just a kernel tagging mechanism. It
is not a feature supported by AMD's drivers. We don't need to rely on
this, as the functions' calling conventions identify kernels. We also
rely on the "sycl-device" module flag to restrict the passes to SYCL
code.

This patch re-uses the existing `TargetHelpers` namespace to hide the
target-specific logic behind a new class: the `KernelCache`. This
provides a way of maintaining a cache of kernels, with optional
annotation metadata (it could be expanded in the future with more types
of payload). It also provides abstracted ways of handling certain RAUW
operations on kernels, though currently only a minimum required to
support the two existing patterns. The aim of this is to hide all
concept of "annotations" from the passes, and make it an implementation
detail of the `KernelCache`.

During this work, it was noticed that our handling of annotations was
incomplete. NVVM annotations are not required to only only have 3
operands, as the official documentation shows. It's actually a list of
pairs, any one of which may declare the function a kernel. Thus we may
have missed valid kernels. Tests have been added to check for this.

The `GlobalOffset` pass was also treating "unsupported" architectures as
AMDGPU architectures, so that has been tightened up and the tests have
been updated to ensure they actually register as AMD modules.

LIT tests have been cleaned up somewhat, to remove unnecessary features
like comments and function linkage types.

Several LIT tests have been converted to use
the `update_test_checks.py` or `update_llc_test_checks.py` scripts,
where appropriate. These tools cannot currently emit checks for named
metadata nor certain assembly features, so some tests must remain as
they are.
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.

7 participants