minior update to test

clang-format

comments and fix typo

In D124418#3473558, @ChuanqiXu wrote:

Hi, could you elaborate more what CHR does and why we need to skip coro.id? I don't know what happened now

fix typo

This issue has been blocking our internal module re-enablement for some time now, and we really appreciate any feedback. We also wonder if only DeducedTemplateSpecializationType is affected or it could also happen to other types.

In D109175#2989997, @yaxunl wrote:

In D109175#2989823, @weiwang wrote:

In D109175#2987048, @jdoerfert wrote:

In D109175#2986782, @yaxunl wrote:

I agree with Johannes and Alexey that deferred diags are only needed when LangOpts.OMPTargetTriples.empty(). However, I am not sure whether it is only needed in device compilation.

For other offloading languages like CUDA/HIP it is needed in both device and host compilation.

Technically, we might even want to delay in host only mode for OpenMP, but that is something we can revisit (e.g., by dynamically setting a flag based on the directives we've seen).
@yaxunl Should we for now check if there is any associated offload job?

Shall we go ahead and get this change in and think about more longer term solution later?

LGTM. This patch should be sufficient to limit deferred diags to OpenMP with offloading. Device compilation is covered by OpenMPIsDevice and host compilation is covered by !LangOpts.OMPTargetTriples.empty(). I will leave the decision to Johannes.

In D109175#2987048, @jdoerfert wrote:

In D109175#2986782, @yaxunl wrote:

I agree with Johannes and Alexey that deferred diags are only needed when LangOpts.OMPTargetTriples.empty(). However, I am not sure whether it is only needed in device compilation.

For other offloading languages like CUDA/HIP it is needed in both device and host compilation.

Technically, we might even want to delay in host only mode for OpenMP, but that is something we can revisit (e.g., by dynamically setting a flag based on the directives we've seen).
@yaxunl Should we for now check if there is any associated offload job?

update as discussed.

In D109175#2980900, @jdoerfert wrote:

In D109175#2980806, @weiwang wrote:

In D109175#2980744, @jdoerfert wrote:

Why do we need this flag, is the absence of -fopenmp-targets not sufficient?

Just double checked, this is the full omp related options currently in use:

"-fopenmp"
"-fopenmp-version=31"
"-fopenmp-version=31"
"-fopenmp-cuda-parallel-target-regions"

We saw a huge number of DECLS_TO_CHECK_FOR_DEFERRED_DIAGS records. I don't know if this has anything to do with omp version being 31, since prior 5.0, everything is available on host.

I don't think we are selective right now. As I was saying, disable deferred parsing if fopenmp-targets is missing, no need for this option.

In D109175#2980744, @jdoerfert wrote:

Why do we need this flag, is the absence of -fopenmp-targets not sufficient?

Just double checked, this is the full omp related options currently in use:

"-fopenmp"
"-fopenmp-version=31"
"-fopenmp-version=31"
"-fopenmp-cuda-parallel-target-regions"

We saw a huge number of DECLS_TO_CHECK_FOR_DEFERRED_DIAGS records. I don't know if this has anything to do with omp version being 31, since prior 5.0, everything is available on host.

In D109175#2980446, @lebedev.ri wrote:

In D109175#2980333, @weiwang wrote:

Our internal codebase never uses the target directive. Once the deferred diags is bypassed, we observed 18% e2e build time improvement.

Is that with -fopenmp or without?
That seems, kinda a lot more than i would have expected,
perhaps there are some other ways to reduce the overhead other than this approach?

Our internal codebase never uses the target directive. Once the deferred diags is bypassed, we observed 18% e2e build time improvement.

In D70172#2964118, @sugak wrote:

Hi @yaxunl! I'm working on upgrading a large codebase from LLVM-9 to LLVM-12. I noticed on average 10% compilation speed regression that seems to be caused this change. We use Clang modules and historically provide -fopenmp compiler flag by default. The problem seems to be that compiling and importing modules is now slower, with the generated modules size increased by 2X. llvm-bcanalyzer tool shows that it's dominated by DECLS_TO_CHECK_FOR_DEFERRED_DIAGS. If I understand it right, your change is only relevant when target offloading is used. I inspected all of #pragma omp directives and can confirm that we don't use it.

I see that most of this code is gated by OpenMP flag. I wonder if there is a finer grain way to enable openmp parallel code generation without target offloading? Would it make sense to extend this code to check if -fopenom-targets is set before recording DECLS_TO_CHECK_FOR_DEFERRED_DIAGS?

Note, this was measured with @weiwang's https://reviews.llvm.org/D101793.

In D101793#2772717, @yaxunl wrote:

In D101793#2772461, @weiwang wrote:

Thanks for the approval!

Just want to understand the list of "decls to check for deferred diagnostics" better, where are these decls coming from? And why do they need to be checked for warnings? I see decls from libc are in the list, but I have no idea why are they selected.

For offloading languages e.g. OpenMP/CUDA/HIP, there are apparent errors in functions shared between host and device. However, unless these functions are sure to be emitted on device or host, these errors should not be emitted. These errors are so called deferred error messages. The function decls which need to be checked are recorded. After AST is finalized, the AST of these functions are iterated. If a function is found sure to be emitted, the deferred error message in it are emitted.

Thanks for the approval!

make both ASTReader::DeclsToCheckForDeferredDiags and Sema::DeclsToCheckForDeferredDiags SmallSetVector

In D101793#2772021, @yaxunl wrote:

In D101793#2769297, @weiwang wrote:

Tried to make Sema::DeclsToCheckForDeferredDiags llvm::SmallSetVector. The heap RSS did drop significantly (from peak 100GB to 59GB) , but not as good as the current fix (peak 26GB), which makes ASTReader::DeclsToCheckForDeferredDiags llvm::SmallSetVector.

I think the reason is that the duplicated decls are read from multiple module file sources (ASTReader::ReadAST() -> ASTReader::ReadASTBlock()), then stored into ASTReader::DeclsToCheckForDeferredDiags, then goes into Sema::DeclsToCheckForDeferredDiags in ASTReader::ReadDeclsToCheckForDeferredDiags(). Doing dedup at the early stage when the decls were just read in ASTReader is more effective at reducing RSS.

What if you use SmallSetVector for both Sema::DeclsToCheckForDeferredDiags and ASTReader::DeclsToCheckForDeferredDiags? Does it cause extra memory usage compared to using it only for ASTReader::DeclsToCheckForDeferredDiags? Thanks.

Tried to make Sema::DeclsToCheckForDeferredDiags llvm::SmallSetVector. The heap RSS did drop significantly (from peak 100GB to 59GB) , but not as good as the current fix (peak 26GB), which makes ASTReader::DeclsToCheckForDeferredDiags llvm::SmallSetVector.

In D101793#2760639, @yaxunl wrote:

I think the root cause might be duplicated decls are added to Sema::DeclsToCheckForDeferredDiags defined in

https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Sema/Sema.h#L1789

When compiling source codes, a decl is added only once. However if modules are imported, duplicate decls may be added.

We need to avoid adding duplicate decls to Sema::DeclsToCheckForDeferredDiags. However we cannot simply change it to a set since the order is important, otherwise the error message for later code may show up earlier, causing confusion for users. I would suggest to change its type to SetVector, which keeps the order and also avoids duplicates.

Finally dealt with the other issues I need to take care. Let's resume the discussion.

LGTM.

Thanks for helping on this issue!

In D101509#2749710, @vzakhari wrote:

Yes, you can disable the device RTL build using this control.

It looks like your scripts define one of these PATH variables. And it has something to do with -ffreestanding. I was able to reproduce it with the following example:

#include <stdint.h>

uint64_t foo() {
  return UINT64_C(0x1);
}

$ CPLUS_INCLUDE_PATH=<path>/x86_64-linux-gnu/9.2.0/include clang test.cpp -c -ffreestanding

test.cpp:4:10: error: use of undeclared identifier '__UINT64_C'
  return UINT64_C(0x1);
         ^
<path>/x86_64-linux-gnu/9.2.0/include/stdint-gcc.h:254:21: note: expanded from macro 'UINT64_C'
#define UINT64_C(c) __UINT64_C(c)
                    ^
1 error generated.

I am not sure whether clang is supposed to work in this conditions or not, but the error is not specific to amdgcn device RTL build. Basically, any target that uses the in-tree clang may fail the same way (e.g. LIT tests).

Looks like -DLIBOMPTARGET_AMDGCN_GFXLIST="" would disable Device RTL build, but still build rest of the libomptarget.

In D101509#2749565, @vzakhari wrote:

I have a guess that clang-resource-headers are not built at the point, where we invoke clang. Can you please check this workaround in openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt?

add_custom_command(
  OUTPUT ${bc1_filename}
  COMMAND ${cu_cmd} ${file} -o ${bc1_filename}
  DEPENDS ${file} ${h_files} clang-resource-headers)

I added clang-resource-headers dependency after ${h_files}.

In D101509#2749559, @vzakhari wrote:

@weiwang, I hope you do not mind if I ask you to run some experiments on your side? Otherwise, I am not sure how to proceed :)

Can you please run the command that fails, pass -E to it and check where the header files are coming from? I.e. run this:

cd /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/projects/openmp/libomptarget/deviceRTLs/amdgcn && /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/bin/clang-13 -xc++ -c -std=c++14 -ffreestanding -target amdgcn-amd-amdhsa -emit-llvm -Xclang -aux-triple -Xclang x86_64-unknown-linux-gnu -fopenmp -fopenmp-cuda-mode -Xclang -fopenmp-is-device -D__AMDGCN__ -Xclang -target-cpu -Xclang gfx700 -fvisibility=default -Wno-unused-value -nogpulib -O2 -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/include -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip -E

Regarding your question "how this change would change anything", can you please check for "Not building AMDGCN device RTL: AOMP not found" message in your "old" builds? I suppose my change for find_package invocation might have caused different behavior in your setup. Before my change we were looking for LLVM in the following paths:

$ENV{AOMP}
$ENV{HOME}/rocm/aomp
/opt/rocm/aomp
/usr/lib/rocm/aomp
${LIBOMPTARGET_NVPTX_CUDA_COMPILER_DIR}
${LIBOMPTARGET_NVPTX_CUDA_LINKER_DIR}
${CMAKE_CXX_COMPILER_DIR}

Not we look for LLVM in all paths that cmake examines by default: https://cmake.org/cmake/help/latest/command/find_package.html#search-procedure

In D101509#2749429, @vzakhari wrote:

In D101509#2749382, @weiwang wrote:

We are getting build errors internally with this change. They are all related to libomptarget. Our internal build script uses gcc to build llvm.

One example:

[108/122] Generating target_impl.gfx700.bc
FAILED: projects/openmp/libomptarget/deviceRTLs/amdgcn/target_impl.gfx700.bc
cd /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/projects/openmp/libomptarget/deviceRTLs/amdgcn && /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/bin/clang-13 -xc++ -c -std=c++14 -ffreestanding -target amdgcn-amd-amdhsa -emit-llvm -Xclang -aux-triple -Xclang x86_64-unknown-linux-gnu -fopenmp -fopenmp-cuda-mode -Xclang -fopenmp-is-device -D__AMDGCN__ -Xclang -target-cpu -Xclang gfx700 -fvisibility=default -Wno-unused-value -nogpulib -O2 -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/common/include -I/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs /home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip -o target_impl.gfx700.bc
/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip:185:25: error: use of undeclared identifier '__UINT64_C'
  lo = (uint32_t)(val & UINT64_C(0x00000000FFFFFFFF));
                        ^
/home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include/stdint-gcc.h:254:21: note: expanded from macro 'UINT64_C'
#define UINT64_C(c) __UINT64_C(c)
                    ^
/home/wangwei/local/llvm-project/openmp/libomptarget/deviceRTLs/amdgcn/src/target_impl.hip:186:26: error: use of undeclared identifier '__UINT64_C'
  hi = (uint32_t)((val & UINT64_C(0xFFFFFFFF00000000)) >> 32);
                         ^
/home/wangwei/fbsource/fbcode/third-party2/gcc/9.x/centos7-native/3bed279/lib/gcc/x86_64-redhat-linux-gnu/9.x/include/stdint-gcc.h:254:21: note: expanded from macro 'UINT64_C'
#define UINT64_C(c) __UINT64_C(c)
                    ^
2 errors generated.

Hello @weiwang, thank you for reporting this. Can you please provide some details how to reproduce it?

One strange thing is that the pre-built /data/users/wangwei/tp2/llvm-build/platform009/build_nopic/bin/clang-13 includes stdint-gcc.h, where it takes the definition for UINT64_C. In my builds clang takes the definition from its own lib/clang/13.0.0/include/stdint.h.

Thanks for the quick response. It may not be easily reproducible since the build script that triggers this sets up its own environment. This is part of the company's internal build system. During my local try, clang built clang always works, but the build script uses gcc to build clang. Maybe gcc would insert its own library headers into search path, and this could cause some confusing about the order of include paths? But again, we have always used gcc to build clang, and it never had issue until now. I am not sure how this change would change anything.

If you do not need libomptarget for your package, you may pass -DOPENMP_ENABLE_LIBOMPTARGET=OFF to cmake.

With -DOPENMP_ENABLE_LIBOMPTARGET=OFF, the error is gone. I'll check internally to see if libomptarget can be disabled. Meanwhile, it would still be great to know what went wrong.

We are getting build errors internally with this change. They are all related to libomptarget. Our internal build script uses gcc to build llvm.

In D101793#2735336, @yaxunl wrote:

Decls in Sema::DeclsToCheckForDeferredDiags is supposed to be unique. Therefore the fact that '1,734,387,685 out of 1,734,404,000 elements are the same' is surprising. Did this happen when you compile the source code and write AST? What language was the source code? C++, OpenMP, or CUDA? What was the decl that got duplicated? Thanks.

We've seen a huge memory footprint from AST Reader/Writer in a single CU with module enabled from internal workloads. Upon further analysis, the content of vector DeclsToCheckForDeferredDiags seems mostly redundant. In one case, 1,734,387,685 out of 1,734,404,000 elements are the same. While this may indicate something wrong with the source itself, it also suggests that compiler would be better to perform deduplication on this type of Decl ID.

update comment and remove one unintended change.

fixed test failures due to bit check

1. serialize and de-serialize the flag.
2. update related test cases.

update according to comment.

Use the propagation approach.

The overhead of repeatedly scanning all summaries could be quite high in the current ValueInfo::isDSOLocal implementation, especially in large projects. This change helped reduce overall thinLTO link time of a major internal workload by 20 mins.

Ah, I am sorry. Thanks for fixing it.

In D85808#2424588, @tejohnson wrote:

lgtm with a couple of minor nits noted below that you can fix before submitting

1. Fix typo.
2. Minor order adjustment in testcase.

rebase

rebase

@tejohnson @MaskRay Do you have other comments?

@tejohnson @MaskRay Do you have other comments?

In D85808#2403587, @tejohnson wrote:

Thanks for adding the Driver test. I was thinking of something to test the CompilerInvocation changes, similar to your test using opt, that ensures the option has the desired behavior when invoked via clang. Looks like there is an existing test clang/test/Frontend/optimization-remark-with-hotness.c that perhaps could be extended or leveraged?

1. Add clang test with remarks output.
2. Fix a missing dependency on PSI in legacy pass manager.

revert back MSVC fix

update to address MSVC build error

update test case

update test case for clang option pass-through

Thanks! I will wait for his input.

@MaskRay @tejohnson, do you have other comments regarding this change and its dependent?

add test case