This is an archive of the discontinued LLVM Phabricator instance.

clang/OpenCL: Apply default attributes to enqueued blocks
ClosedPublic

Authored by arsenm on Jan 12 2023, 8:52 AM.

Details

Reviewers
Anastasia
yaxunl
Summary

This was missing important environment context, like denormal-fp-math
and target-features. Curiously this seems to be losing nounwind. Note
this only fixes the actual invoke kernel. The invoke function is
already setting the default attribute set for internal
functions. However that is still buggy since it's not applying any use
function attributes (it's also missing uniform-work-group-size).

There seem to be too many different functions for setting attributes
with inconsistent behavior. The Function overload of
addDefaultFunctionAttributes seems to miss the target-cpu and
target-features. The AttrBuilder one seems to miss optnone (but that
seems to be disallowed on blocks anyway). Neither one calls
setTargetAttributes, when it probably should. uniform-work-group-size
is also set through AMDGPU code when it should be emitting generically
as a language property.

I also noticed update_cc_test_checks for attributes seem to not
connect the captured attribute variables to the attributes at the end
(although I think the numbers happen to work out correctly).

Diff Detail

Event Timeline

arsenm created this revision.Jan 12 2023, 8:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 12 2023, 8:52 AM
arsenm requested review of this revision.Jan 12 2023, 8:52 AM
yaxunl added inline comments.Jan 12 2023, 2:38 PM
clang/lib/CodeGen/TargetInfo.cpp
12487

The kernel should inherit attributes from the parent function since the parent may have function attributes explicitly specified for itself only. The kernel should take the default function attributes.

arsenm added inline comments.Jan 12 2023, 2:43 PM
clang/lib/CodeGen/TargetInfo.cpp
12487

I was specifically try to get those, e.g. the attribute(target) example. Is there some way to specify attributes directly on a block?

yaxunl added inline comments.Jan 12 2023, 8:19 PM
clang/lib/CodeGen/TargetInfo.cpp
12487

I was specifically try to get those, e.g. the attribute(target) example. Is there some way to specify attributes directly on a block?

I don't think so https://godbolt.org/z/Pahv46dh6

arsenm added inline comments.Jan 13 2023, 5:08 AM
clang/lib/CodeGen/TargetInfo.cpp
12487

It doesn't allow noinline or target, but does allow const so it seems to be specific attributes. I guess that means it should just use the defaults

arsenm added inline comments.Jan 13 2023, 5:53 AM
clang/lib/CodeGen/TargetInfo.cpp
12487

Something is inconsistent. If I apply the target attribute to the caller, it allows me to call the target builtin in the block. If I don't, it doesn't allow me to use it in the caller but does in the block. There's a missing diagnostic at least

arsenm updated this revision to Diff 488978.Jan 13 2023, 6:11 AM
arsenm retitled this revision from clang/OpenCL: Make enqueued blocks inherit the parent attributes to clang/OpenCL: Apply default attributes to enqueued blocks.
arsenm edited the summary of this revision. (Show Details)
yaxunl added inline comments.Jan 13 2023, 10:49 AM
clang/test/CodeGenOpenCL/amdgpu-enqueue-kernel.cl
53–57

why this test is removed?

arsenm added inline comments.Jan 13 2023, 10:55 AM
clang/test/CodeGenOpenCL/amdgpu-enqueue-kernel.cl
53–57

Feels redundant unless we're propagating target-features

Anastasia added inline comments.Jan 15 2023, 12:45 PM
clang/test/CodeGenOpenCL/cl20-device-side-enqueue-attributes.cl
26

ATTR0 seems to be set but never used.

Do we need to be checking the output of the whole function body?

I imagine checking the function definition and attributes should be enough for this patch?

arsenm added inline comments.Jan 15 2023, 1:36 PM
clang/test/CodeGenOpenCL/cl20-device-side-enqueue-attributes.cl
26

Yes, but I don't think update_cc_test_checks has that option

yaxunl added inline comments.Jan 30 2023, 6:03 AM
clang/test/CodeGenOpenCL/amdgpu-enqueue-kernel.cl
381

these attributes are not really checked since ATTR1 etc are not used to check the attributes definitions. Is this a bug of update_cc_test_checks.py ? Or it is expected to be manually edited to enforce the checking?

arsenm added inline comments.Jan 30 2023, 7:40 AM
clang/test/CodeGenOpenCL/amdgpu-enqueue-kernel.cl
381

Seems to be an update_cc_test_checks bug I mentioned in the description

yaxunl accepted this revision.Jan 30 2023, 8:51 AM

LGTM. Thanks

This revision is now accepted and ready to land.Jan 30 2023, 8:51 AM