This is an archive of the discontinued LLVM Phabricator instance.

[HIP] emit macro `__HIP_NO_IMAGE_SUPPORT`
ClosedPublic

Authored by yaxunl on May 24 2023, 10:08 AM.

Details

Summary

HIP texture/image support is optional as some devices
do not have image instructions. A macro __HIP_NO_IMAGE_SUPPORT
is defined for device not supporting images (https://github.com/ROCm-Developer-Tools/HIP/blob/d0448aa4c4dd0f4b29ccf6a663b7f5ad9f5183e0/docs/reference/kernel_language.md?plain=1#L426 )

Currently the macro is defined by HIP header based on predefined macros
for GPU, e.g __gfx*__ , which is error prone. This patch let clang
emit the predefined macro.

Diff Detail

Event Timeline

yaxunl created this revision.May 24 2023, 10:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2023, 10:08 AM
yaxunl requested review of this revision.May 24 2023, 10:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 24 2023, 10:08 AM
yaxunl updated this revision to Diff 525637.May 25 2023, 8:37 AM

update tests

You seem to be defining a new subtarget feature without actually defining the underlying feature. I thought the issue was specific missing image instructions, which is already covered by extended-image-insts

clang/lib/Basic/Targets/AMDGPU.h
458

A negative form shouldn’t apply to targets with images, it will break the IR.

arsenm added inline comments.May 25 2023, 9:06 AM
clang/test/OpenMP/amdgcn-attributes.cpp
36 ↗(On Diff #525637)

There’s also no reason to emit these in the IR. Really we should stop emitting most of these

You seem to be defining a new subtarget feature without actually defining the underlying feature. I thought the issue was specific missing image instructions, which is already covered by extended-image-insts

gfx90a does not have extended-image-insts but supports image/texture API's in HIP. Brian suggested to determine whether HIP image is supported based on whether the subtarget has image-insts.

I thought image-insts is a subtarget feature as it is defined in https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/AMDGPU.td#L499

Did I miss something?

yaxunl added inline comments.May 25 2023, 9:48 AM
clang/test/OpenMP/amdgcn-attributes.cpp
36 ↗(On Diff #525637)

I agree.

For example, for image-insts, we do not need to expose it to FE since there is no point of enable/disable it in FE. We only need to know whether the GPU supports image insts or not. In this case, it seems simpler to just get that information through ISA version.

yaxunl updated this revision to Diff 526263.May 27 2023, 6:49 AM

using ISA version to determine whether image is supported

tra added inline comments.May 30 2023, 9:19 AM
clang/lib/Basic/Targets/AMDGPU.cpp
248

My usual nit about negations: !(ISAVer.Major == 9 && ISAVer.Minor == 4) is easier to read.

Is ISA 9.4 the only version w/o image support? Or should it be a range comparison instead?

using ISA version to determine whether image is supported

That’s backward. You can track the feature in clang separately and just not emit it. We do that for a few other features. You just stop the TargetParser parts

using ISA version to determine whether image is supported

That’s backward. You can track the feature in clang separately and just not emit it. We do that for a few other features. You just stop the TargetParser parts

Sorry, I did not quite get it. Do you mean to add a feature to https://github.com/llvm/llvm-project/blob/85670ac86813b170c9301aa477421c56a71a7e1e/llvm/lib/TargetParser/TargetParser.cpp#L107 ? should I add FEATURE_IMAGE or FEATURE_NO_IMAGE ?

arsenm added a comment.Jun 5 2023, 5:02 AM

using ISA version to determine whether image is supported

That’s backward. You can track the feature in clang separately and just not emit it. We do that for a few other features. You just stop the TargetParser parts

Sorry, I did not quite get it. Do you mean to add a feature to https://github.com/llvm/llvm-project/blob/85670ac86813b170c9301aa477421c56a71a7e1e/llvm/lib/TargetParser/TargetParser.cpp#L107 ? should I add FEATURE_IMAGE or FEATURE_NO_IMAGE ?

It's expressed as positive image feature. I thought there was a feature where clang tracks it per-target, but doesn't emit it in the IR in "target-features" but I can't remember which one it is. Is there a way to track the image feature without emitting it?

yaxunl added a comment.Jun 5 2023, 9:56 AM

using ISA version to determine whether image is supported

That’s backward. You can track the feature in clang separately and just not emit it. We do that for a few other features. You just stop the TargetParser parts

Sorry, I did not quite get it. Do you mean to add a feature to https://github.com/llvm/llvm-project/blob/85670ac86813b170c9301aa477421c56a71a7e1e/llvm/lib/TargetParser/TargetParser.cpp#L107 ? should I add FEATURE_IMAGE or FEATURE_NO_IMAGE ?

It's expressed as positive image feature. I thought there was a feature where clang tracks it per-target, but doesn't emit it in the IR in "target-features" but I can't remember which one it is. Is there a way to track the image feature without emitting it?

clang tracks target feature by TargetOptions::FeatureMap ( https://github.com/llvm/llvm-project/blob/7a2b12b05b39bce08987968270b43ca05d9a4836/clang/include/clang/Basic/TargetOptions.h#L62 )

Each target-specific TargetInfo-derived class initializes this map and CodeGen uses it to emit function attributes consistently.

There is no separate data structure to track target features not emitted to function attributes.

If we want to make these target features unchangeable and not emitted to IR, we could add a set for unchangeable target features and each target can initialize it. These features are read-only and will not be emitted to IR, and will cause a warning and be ignored if users try to force enabling or disabling them.

yaxunl updated this revision to Diff 529644.Jun 8 2023, 9:49 AM

warn if users try to enable/disable image-insts and do not emit image-insts as function attributes in IR

arsenm accepted this revision.Jun 13 2023, 5:11 PM
This revision is now accepted and ready to land.Jun 13 2023, 5:11 PM
This revision was landed with ongoing or failed builds.Jun 14 2023, 7:54 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2023, 7:54 PM

I've started seeing these errors when compiling for OpenMP targeting AMDGPU:

$ clang input.c -fopenmp --offload-arch=gfx1030
warning: feature flag '+image-insts' is ignored since the feature is read only [-Winvalid-command-line-argument]

Any suggestions for what might cause this?

I've started seeing these errors when compiling for OpenMP targeting AMDGPU:

$ clang input.c -fopenmp --offload-arch=gfx1030
warning: feature flag '+image-insts' is ignored since the feature is read only [-Winvalid-command-line-argument]

Any suggestions for what might cause this?

That means TargetInfo::initFeatureMap is called with +image-insts. I will take a look.