LGTM, thanks for fixing this

Beyond constructors/destructors I believe an API which we implement through access to dynamic symbols for global variable is clGetProgramBuildInfo(..., CL_PROGRAM_BUILD_GLOBAL_VARIABLE_TOTAL_SIZE, ...) which is defined as "The total amount of storage, in bytes, used by program variables in the global address space."

In D53153#1263835, @yaxunl wrote:

In D53153#1263810, @scott.linder wrote:

In D53153#1263804, @rsmith wrote:

In D53153#1263771, @scott.linder wrote:

The rationale is that -fvisibility only affects the default, and already does not apply in many cases. For example, see the rest of the conditions above the fvisibility check in getLVForNamespaceScopeDecl: when Var->getStorageClass() == SC_Static the linkage is (usually) internal and the visibility is default.

Visibility is meaningless for an internal-linkage declaration.

I think one question is whether OpenCL language semantics allow us to make these visibility determinations; I am going off of the APIs available to access symbols.

-fvisibility is not governed by the OpenCL specification; it's a Clang / GCC extension, and we get to determine its semantics, which generally override the rules from the language model we're implementing.

I'm a little surprised that symbol visibility is even meaningful when compiling GPU code. Can you give some more background on that?

To the extent that visibility is meaningful, it does seem to make sense for kernel to imply default visibility in the same way an attribute would, since it's an unambiguous marker that the function is intended to be called from outside the DSO. It's less obvious to me that the same is true for global variables; is there no distinction in OpenCL between global variables that can be accessed by the host and global variables that are internal to the device-side computation?

This makes more sense, thank you for the explanation. Ignore my last post, it makes sense that visibility is defaulted and ignored when a symbol is assigned internal linkage.

I am still not confident about globals; maybe @b-sumner has more insight? We have APIs for accessing global variable symbols form the host, but this may be AMD-specific, not general to OpenCL.

One thing I can think of the global variables is their initialization. If their initialization must be done by the runtime, then they have to be visible to the runtime.

In D53153#1263804, @rsmith wrote:

In D53153#1263771, @scott.linder wrote:

The rationale is that -fvisibility only affects the default, and already does not apply in many cases. For example, see the rest of the conditions above the fvisibility check in getLVForNamespaceScopeDecl: when Var->getStorageClass() == SC_Static the linkage is (usually) internal and the visibility is default.

Visibility is meaningless for an internal-linkage declaration.

I think one question is whether OpenCL language semantics allow us to make these visibility determinations; I am going off of the APIs available to access symbols.

-fvisibility is not governed by the OpenCL specification; it's a Clang / GCC extension, and we get to determine its semantics, which generally override the rules from the language model we're implementing.

I'm a little surprised that symbol visibility is even meaningful when compiling GPU code. Can you give some more background on that?

To the extent that visibility is meaningful, it does seem to make sense for kernel to imply default visibility in the same way an attribute would, since it's an unambiguous marker that the function is intended to be called from outside the DSO. It's less obvious to me that the same is true for global variables; is there no distinction in OpenCL between global variables that can be accessed by the host and global variables that are internal to the device-side computation?

That's my understanding, at least.

The rationale is that -fvisibility only affects the default, and already does not apply in many cases. For example, see the rest of the conditions above the fvisibility check in getLVForNamespaceScopeDecl: when Var->getStorageClass() == SC_Static the linkage is (usually) internal and the visibility is default. In cases where individual symbols need unique visibility an __attribute__ can be used.

Address feedback

Rebase and remove redundant qualifiers

rL344166

Cleanup patch is at https://reviews.llvm.org/D53088

Ping

Address feedback

I can submit a separate patch to clean up things like making the empty definitions into declarations, removing redundant public: qualifiers, and removing redundant references to this, but I don't want to mix those changes into this patch.

Remove update of dyn_cast in WebAssemblyFixFunctionBitcasts which does not use CallSite

I will update the patch to modify the HIP toolchain and to add tests for global variables.

Address feedback

LGTM

Ping

Update docs

I don't know who else to add as a reviewer; Sam, is there someone else outside of AMD that would be interested in reviewing this?

Address feedback

Remove -debug flag from test

Add test using IndirectCallPromotion

Thanks, Sam. I think the requirements of the two passes are different enough that combining them is not particularly useful.

Move CallPromotionUtils changes to new review (https://reviews.llvm.org/D52792)

Sorry, moved a little fast and grabbed the wrong patch at first.

After reading the WebAsm code more closely it seems like our goals are different. They seem to support indirect calls, but only if certain properties of the functions match (namely number of arguments and return type). For example, the existing pass does not transform anything (e.g. leaves a bitcast function call present) if the bitcast only modifies argument types in a trivial way. For AMDGPU we need to always eliminate the bitcast, and we don't necessarily need to be generating wrapper functions.

The WebAsm code essentially does the same thing, but also creates wrapper functions when the number of arguments differ, or when the return value differs (it seems like this could just be a cast as well). I will move the WebAsm pass to lib/Transform and use it in both backends. I am not sure who from WebAsm to ask to take a look once I'm done?

In D52741#1251534, @mssimpso wrote:

In D52741#1251525, @scott.linder wrote:

@mssimpso Are you OK with reviewing the changes to CallPromotionUtils.cpp in this patch? I wasn't sure who the correct code owner was.

I'm not sure there is a code owner, but the CallPromotionUtils part looks good to me.

@mssimpso Are you OK with reviewing the changes to CallPromotionUtils.cpp in this patch? I wasn't sure who the correct code owner was.

This patch requires changes to CallGraph, and doesn't address inlining through bitcasts. I think https://reviews.llvm.org/D52741 can supercede it, and if we still want to make those changes to CallGraph they can be done independently.

Ping

Ping

Address feedback: make MDT optional, add braces, run clang-format.

It looks like to make this change we need to update CallGraph as well; it currently just uses .getCalledFunction(), so the graph doesn't have the edge created by the bitcasted call.

LGTM, but adding Matt because I think he was the one to originally request eliding defaulted fields. From what Tony described I agree we can't do this generally because of options which depend on attributes, e.g. -mattr=+/-xnack changes the "default".

Seems to be related to the order; this succeeds:

I still see asserts related to bitcasted function calls, but I haven't gotten to the bottom of why; an example:

Update patch to include all changes since the original patch was reverted.

Sorry; I just checked again and llc had not been rebulit for some reason. I deleted it and ran the build again and that example does compile fine.

I still don't understand exactly when a call is indirect, but when an argument is cast this still seems to fail:

Update existing tests RUN lines with -verify-machine-dom-info. This replicates what the expensive tests enabled.

The regression was caught by the expensive_checks build, if that is sufficient for testing? I am actually not sure how to write a lit test for this, because opt -analyze will just re-calculate the DT; I don't know how to just print the DT after the legalize pass runs.

Review for fix of that regression is at https://reviews.llvm.org/D51742

Ping

LGTM, thanks!

Ping

Update test name to be clear it only runs when aarch64 is available.

Added test for shared ownership. I've run it locally with ASan and UBSan enabled.

The use I have in mind is actually an external component which depends on LLVM. We currently implement reading MessagePack metadata there, but when we land the changes to generate it in LLVM it would be nice to avoid duplicating the code there.

+Matt to confirm, but our executable format is a shared object and we eventually want to support preemptible symbols through the PLT. We already generate GOT entries for globals.

I've updated the patch to just support shared_ptr; I think the more generic option is right for now.

Address feedback. There was no particular reason for not writing these inline, other than to prevent typos. With the verifier and tests we have I don't think that is an issue, and I agree these being grepable is worthwhile.

Use MRI.clearKillFlags

Add virtual destructor to base class and clean up tests.

Default the test case destructor rather than give it an empty body.

In cleaning up this patch I noticed our YAML parser does not handle an explicit document containing only a block-level plain scalar, e.g.:

Clear killed flag on uses, add test at -O0

Rebase and ping

Modular build and UB fixes (r340507) are committed.

I think even in the simplest case my code is wrong. The $soffset operand to the BUFFER_LOAD remains killed but the def is not moved into the LoopBB. I am essentially transforming:

Thank you both! I will try to keep modules in mind in the future.