Page MenuHomePhabricator

[OpenCL] Add pure attributes to vload builtins
ClosedPublic

Authored by stuart on Sep 29 2021, 11:08 AM.

Details

Summary

Use the pure attribute for the vload, vload_half and vloada_half builtins.

Diff Detail

Unit TestsFailed

TimeTest
70 msx64 debian > Clang.SemaOpenCL::fdeclare-opencl-builtins.cl
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/clang -cc1 -internal-isystem /var/lib/buildkite-agent/builds/llvm-project/build/lib/clang/14.0.0/include -nostdsysteminc /var/lib/buildkite-agent/builds/llvm-project/clang/test/SemaOpenCL/fdeclare-opencl-builtins.cl -triple spir -verify -pedantic -Wconversion -Werror -fsyntax-only -cl-std=CL -fdeclare-opencl-builtins -DNO_HEADER
70 msx64 debian > LLVM.Bindings/Go::go.test
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/llvm-go go=/usr/bin/go test llvm.org/llvm/bindings/go/llvm

Event Timeline

stuart created this revision.Sep 29 2021, 11:08 AM
stuart requested review of this revision.Sep 29 2021, 11:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2021, 11:08 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
svenvh added a comment.Oct 7 2021, 8:52 AM

For the constant address space, the const attribute (or readnone) can be used. As memory in the constant address space is immutable, the statement in the langref that: "if a readnone function reads or writes memory visible to the program, or has other side-effects, the behavior is undefined" does not apply. The reading of immutable memory does not have side-effects, nor can it be affected by side-effects.

I think readnone might be too strong, because the pointer argument will still be dereferenced (while readnone implies that "the function computes its result [...] based strictly on its arguments, without dereferencing any pointer arguments").

stuart added a comment.Oct 8 2021, 3:46 AM

For the constant address space, the const attribute (or readnone) can be used. As memory in the constant address space is immutable, the statement in the langref that: "if a readnone function reads or writes memory visible to the program, or has other side-effects, the behavior is undefined" does not apply. The reading of immutable memory does not have side-effects, nor can it be affected by side-effects.

I think readnone might be too strong, because the pointer argument will still be dereferenced (while readnone implies that "the function computes its result [...] based strictly on its arguments, without dereferencing any pointer arguments").

That may be so, but the function does not have its own side-effects, nor can it depend upon any side-effects, as the memory in question is truly immutable (i.e. it cannot even be changed by another process, thread, or shared memory interface in such a way that would be observable).

All functions, with or without __attribute__((const)) can be said to require memory reads in the form of the instruction fetches needed to execute the instructions that comprise the compiled function. It is also quite normal for constant pools to be generated for a function, from which memory reads will be performed despite no pointer being dereferenced in the original C source.

Both __attribute__((const)) and readnone are intended to permit arbitrary code motion, and so in my opinion the langref misstates the true nature of the readnone attribute. (The Clang manual does not even document __attribute__((const)).) From a cursory examination of the LLVM source, I did not find any use of readnone that conflicts with this interpretation of the readnone attribute.

Does the langref need to be amended, first, or is it okay to interpret the readnone attribute as it was clearly intended, without going through the process of updating the langref first?

I can update this review to use __attribute__((pure)) for all address spaces, for the time being, but it seems a shame that the poor wording in the langref might (necessarily) prevent us from making the optimal change.

Does the langref need to be amended, first, or is it okay to interpret the readnone attribute as it was clearly intended, without going through the process of updating the langref first?

I can update this review to use __attribute__((pure)) for all address spaces, for the time being, but it seems a shame that the poor wording in the langref might (necessarily) prevent us from making the optimal change.

Apologies for the late reply... I'd prefer to get the langref updated first, for the sake of consistency and to ensure other stakeholders agree with the interpretation. You can still go ahead with the __attribute__((pure)) changes of course.

stuart updated this revision to Diff 394580.Dec 15 2021, 8:26 AM
stuart retitled this revision from [OpenCL] Add pure and const attributes to vload builtins to [OpenCL] Add pure attributes to vload builtins.
stuart edited the summary of this revision. (Show Details)

Apologies for the late reply... I'd prefer to get the langref updated first, for the sake of consistency and to ensure other stakeholders agree with the interpretation. You can still go ahead with the __attribute__((pure)) changes of course.

Thanks! I have updated the review to use __attribute__((pure)) only (i.e. it no longer uses __attribute__((const)).

svenvh accepted this revision.Dec 16 2021, 4:07 AM

Thanks! I have updated the review to use __attribute__((pure)) only (i.e. it no longer uses __attribute__((const)).

LGTM!

This revision is now accepted and ready to land.Dec 16 2021, 4:07 AM
This revision was landed with ongoing or failed builds.Dec 16 2021, 6:55 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Dec 16 2021, 7:11 AM

looks like this broke tests: http://45.33.8.238/linux/63308/step_7.txt

Please take a look, and revert for now if it takes a while to fix.

stuart reopened this revision.Dec 16 2021, 7:18 AM
This revision is now accepted and ready to land.Dec 16 2021, 7:18 AM
stuart updated this revision to Diff 394878.Dec 16 2021, 7:33 AM

I've updated the review to include test changes that are required for check-clang-semaopencl to pass.

Thanks! Bots look happy again :)

svenvh accepted this revision.Dec 16 2021, 9:01 AM
This revision was landed with ongoing or failed builds.Dec 16 2021, 10:31 AM
This revision was automatically updated to reflect the committed changes.