In D77583#1965823, @yaxunl wrote:

Is the runtime and HIP directed test change in place?

Rebase to the latest trunk.

In D77245#1957290, @dstuttard wrote:

My fix has been hanging around for a little while now - so my memory of it is a little hazy.

However, the issue I saw (and attempted to fix) was similar to yours. Agreed that I think this is a rare occurrence, so perhaps your approach makes more sense (and is simpler/cheaper).
One question - the reason I went with the solution I did was that a previous iteration of the loop doing the optimisation was invalidating the iterator stored in a later list - which caused a crash - is your fix working around this by avoiding creating those potentially dependent lists in the first place?? (I wasn't 100% sure about the logic).

I guess one potential advantage with my approach is that you still get the potential optimisations at the expense of having to re-compute the lists for dependent instructions.

In D59321#1955646, @arsenm wrote:

In D59321#1955405, @hliao wrote:

Do we have a better way to avoid adding those empty bitcode files?

No, we need the files to exist for tests. This is what existing bitcode link tests do

this patch breaks the test clang/test/Headers/nvptx_device_cmath_functions.cpp, could you please fix it ASAP?

Do we have a better way to avoid adding those empty bitcode files?

In D77154#1953423, @MaskRay wrote:

This seems fine. I have -DCMAKE_EXE_LINKER_FLAGS=-Wl,--gdb-index -DCMAKE_SHARED_LINKER_FLAGS=-Wl,--gdb-index -DLLVM_USE_SPLIT_DWARF=On in my long cmake command line. I am just wondering whether this new option will be discoverable.

Some people don't use -DLLVM_USE_LINKER=lld, but use -DLLVM_ENABLE_LLD=On.

The same patch posted previously, https://reviews.llvm.org/D58628

In D76365#1947462, @hliao wrote:

In D76365#1947103, @tra wrote:

In D76365#1946938, @hliao wrote:

The new revision is accepted, right? Just want to confirm as it seems you accept it before I posted the new change.

The approval was for the old version. I didn't undo it when I reopened the review. The diff looks OK, though the last variant still leaves open the question of what's the meaning of these attributes and what are the restrictions on their use.

So what's the reasonable thing to do if I write something like this:

__attribute__((device_builtin_surface_type)) int foo; // Ignore? Warn? Error? Do something sensible?

For such case, NVCC reports the following error:

kernel.cu(3): error: attribute "device_builtin_surface_type" does not apply here

1 error detected in the compilation of "kernel.cpp1.ii"

That error is generated after nvcc --keep -g -c kernel.cu from this sample code (kernel.cu)

#include <cuda.h>

__attribute__((device_builtin_surface_type)) int foo;

int f() {
  return foo;
}

I changed that sample code a little bit to this one

#include <cuda.h>

#if 1
typedef __attribute__((device_builtin_surface_type)) int dev_texsurf_int_t;
dev_texsurf_int_t foo;
#else
__attribute__((device_builtin_surface_type)) int foo;
#endif

int f() {
  return foo;
}

It triggers a crash in NVCC with the same compilation command line.

We may enhance clang to report an error instead of a warning only so far.

In D76365#1947103, @tra wrote:

In D76365#1946938, @hliao wrote:

The new revision is accepted, right? Just want to confirm as it seems you accept it before I posted the new change.

The approval was for the old version. I didn't undo it when I reopened the review. The diff looks OK, though the last variant still leaves open the question of what's the meaning of these attributes and what are the restrictions on their use.

So what's the reasonable thing to do if I write something like this:

__attribute__((device_builtin_surface_type)) int foo; // Ignore? Warn? Error? Do something sensible?

In D76365#1947103, @tra wrote:

In D76365#1946938, @hliao wrote:

The new revision is accepted, right? Just want to confirm as it seems you accept it before I posted the new change.

The approval was for the old version. I didn't undo it when I reopened the review. The diff looks OK, though the last variant still leaves open the question of what's the meaning of these attributes and what are the restrictions on their use.

So what's the reasonable thing to do if I write something like this:

__attribute__((device_builtin_surface_type)) int foo; // Ignore? Warn? Error? Do something sensible?

In D76365#1946925, @hliao wrote:

Fix Sema checks on partial template specialization.

• Revise Sema checks on the template class.

Fix Sema checks on partial template specialization.

In D76948#1946861, @tra wrote:

Would it be possible to update the old review with the new diff? It would make it easier to see the incremental changes you've made. If the old review can be reopened that would be great as it would keep all relevant info in one place, but I'm fine doing the review here, too, if phabricator does not let you do it.

In D76948#1946861, @tra wrote:

Would it be possible to update the old review with the new diff? It would make it easier to see the incremental changes you've made. If the old review can be reopened that would be great as it would keep all relevant info in one place, but I'm fine doing the review here, too, if phabricator does not let you do it.

This's revised change from https://reviews.llvm.org/D76365 after fixing Sema checks on the template partial specialization. With this change, I could compile the following sample code using surface reference.

In D76365#1946407, @tra wrote:

In D76365#1946345, @tra wrote:

Looks like the change breaks compilation for us:

In file included from <built-in>:1:
In file included from llvm_unstable/toolchain/lib/clang/google3-trunk/include/__clang_cuda_runtime_wrapper.h:104:
In file included from cuda/include/cuda_runtime.h:116: cuda/include/cuda_surface_types.h:91:42: error: illegal device builtin surface reference type 'surface<void, dim>' declared here
struct  __device_builtin_surface_type__  surface<void, dim> : public surfaceReference
                                         ^
cuda/include/cuda_surface_types.h:91:42: note: 'surface<void, dim>' needs to be instantiated from a class template with the 2nd template argument as an integral value
1 error generated when compiling for sm_60.

I'm investigating, but we may need to roll back this patch. Stay tuned.

It appears that the assumptions of what types the attributes can apply to are not valid. In CUDA headers they are also used on non-templated classes/structs. E.g in cuda/include/cuda_surface_types.h:74

struct __attribute__((device_builtin_surface_type)) surface : public surfaceReference
{
...
};

I'll undo this patch until we can make it work.

In D76365#1946345, @tra wrote:

Looks like the change breaks compilation for us:

In file included from <built-in>:1:
In file included from llvm_unstable/toolchain/lib/clang/google3-trunk/include/__clang_cuda_runtime_wrapper.h:104:
In file included from cuda/include/cuda_runtime.h:116: cuda/include/cuda_surface_types.h:91:42: error: illegal device builtin surface reference type 'surface<void, dim>' declared here
struct  __device_builtin_surface_type__  surface<void, dim> : public surfaceReference
                                         ^
cuda/include/cuda_surface_types.h:91:42: note: 'surface<void, dim>' needs to be instantiated from a class template with the 2nd template argument as an integral value
1 error generated when compiling for sm_60.

I'm investigating, but we may need to roll back this patch. Stay tuned.

In D76365#1944272, @tra wrote:

LGTM. Next step is to figure out what various __nv_tex_surf_handler(<string>...) maps to for various strings (there are ~110 of them in CUDA-10.2) and implement its replacement. I think we should be able to do it in the wrapper file.

Rebase to the master code

Fix windows build and revise header change.

Add Sema checks on CUDA device builtin surface/texture attributes.

Minor revising following reviewer's comment. Work on Sema checks and upload another review.

In D76365#1932517, @tra wrote:

In D76365#1932439, @hliao wrote:

That's a magic. I could not figure out how it works. From its use, e.g. tex2D on texture<T, cudaTextureType2D, cudaReadModeElementType>,

__nv_tex_surf_handler("__tex2D_v2", (typename __nv_tex_rmet_cast<T>::type) &temp, t, x, y);

__tex2D_v2 is a string literal. However, it's more likely a underly function name for the real implementation. Hardly imagine that that string literal is checked directly instead used for constructing the real function name. If that's the case, we also need to find that where that underlying functions are defined as the device bitcode library has no such definition.

Most likely it's a compiler built-in with no implementation we could reuse and we'll need to implement our own. It should be fairly straightforward to figure out what it does by compiling all variants used by CUDA headers and observing generated PTX. The first 'meta' argument may be tricky, but we should be able to retrieve the constant string value in the front-end and map it to appropriate intrinsic or generate necessary glue.

In D76365#1932398, @tra wrote:

I believe LLVM does have nvvm.texsurf.handle implemented: https://github.com/llvm/llvm-project/blob/d9972f848294b06807c8764615852ba2bc1e8a74/llvm/include/llvm/IR/IntrinsicsNVVM.td#L1150

In D76365#1932392, @tra wrote:

Note that, clang-based one needs defining texture fetch functions as they could not be reused from CUDA SDK. That part is enclosed with #if defined(clang).

What prevents clang to compile the texture functions in the CUDA headers? It looks like we'll need to implement the __nv_tex_surf_handler() builtin, but other than that it should work.

With this revision, the following sample could be compiled with CUDA SDK and almost the same PTX code is generated.

More refinement to compile sample code with CUDA headers.

Revise one part of the logic to reduce condition evaluation overhead.

Fix more clang-tidy warnings.

Fix warnings from clang-tidy.

Fix warnings from clang-tidy.

Reformatting with clang-format.

Rebase to the latest trunk.

BTW, why that variable cannot have an initializer? Suppose that initializer is a trivial one, initializing to 0, would that cause any issue in the compilation?

Remove unncessary condition checking.

the assertion in addUsedGlobal should not be removed. that will remove the guard for potential bugs

invariant checking already takes account of loading from constant address space or memory (AA::pointsToConstantMemory), that's almost equivalent to adding invariant attributes. Why do we mark these constant loads with additional attributes?

That makes sense to me. At least, in release build, we won't generate
unnecessary files.

Think again. There's no need to tell the languages for adding that compile option.

Apply to objc and objcxx.

Fix pre-merge checks.

@rjmccall @rsmith @tra, could you review on this revision?

Rebase to the latest trunk.

In D75135#1892257, @sbc100 wrote:

Sounds reasonable. We can looking into other solutions.

My understand is that the IR in question is perfectly valid, and can be compiled on other targets, can't it? Its only WebAssembly that can't compile this. We can't ask the producer so not produce this valid bitcode when building for WebAssembly can we?

It seems incorrect for me as the input IR has an alias casting function pointer to data pointer. That symbol itself should be regarded as a function symbol any more. If that IR needs to pass compilation, it should correct its generation by replacing that casting with one casting between function pointers. The casting from a function pointer to data pointer could be done within the accessor function.

Removed from reviewer list as the new patch takes all target-specific approach.

Rebase to the trunk.

Rebase to the trunk.

Revise following reviewer comments.

Rebase the code to the latest trunk.

Skip non-function or non-TU context so far as more cases need considering.

Rebase to the latest trunk code.

+ @rjmccall

one header is missing and breaks the build