revised by Matt's comments.

capitalize macro

update patch with full context

fix typo

In D84362#2283045, @tra wrote:

In D84362#2282992, @yaxunl wrote:

The fix is for the change in D84364. It has no effect on the change in this review. Are you sure the issue you saw is due to change in this review instead of change in D84364?

Pretty sure. We've bisected the failure specifically to commit ee5519d323571c4a9a7d92cb817023c9b95334cd, before D84364 landed.

In D84362#2282965, @tra wrote:

In D84362#2282890, @yaxunl wrote:

I have a fix for the issue reported in D84364. Would you like to try? Thanks.

I can try it on the internal test that crashed with the patch. I've reopened this review and will pick up the diff once you update it.

In D84362#2279845, @tra wrote:

In D84362#2279688, @tra wrote:

Apparently this patch triggers compiler crashes on some of our code. I'll try to create a reproducer, but it would be great to revert the patch for now.

It's likely the same issue as the one reported in D84364, but we probably triggered it independently.

In D84364#2282458, @hctim wrote:

Not a known issue - no, but MSan doesn't play nice with uninistrumented libraries (including things like libcxx) - and so it can be tricky to ensure your build is properly sanitized, which is why I'd recommend the build script :).

In D84364#2279684, @hctim wrote:

Looks like this patch broke the MSan buildbots, PTAL (repro instructions https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild):

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/46239/steps/check-clang%20msan/logs/stdio

FAIL: Clang :: SemaCUDA/deferred-oeverload.cu (11308 of 26387)
******************** TEST 'Clang :: SemaCUDA/deferred-oeverload.cu' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang -cc1 -internal-isystem /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/clang/12.0.0/include -nostdsysteminc -fcuda-is-device -fsyntax-only -verify=dev,com /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/test/SemaCUDA/deferred-oeverload.cu    -std=c++11 -fgpu-defer-diag
: 'RUN: at line 3';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang -cc1 -internal-isystem /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/clang/12.0.0/include -nostdsysteminc -fsyntax-only -verify=host,com /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/test/SemaCUDA/deferred-oeverload.cu    -std=c++11 -fgpu-defer-diag
--
Exit Code: 77

Command Output (stderr):
--
==41680==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0xddfbf73 in clang::OverloadCandidateSet::CompleteCandidates(clang::Sema&, clang::OverloadCandidateDisplayKind, llvm::ArrayRef<clang::Expr*>, clang::SourceLocation, llvm::function_ref<bool (clang::OverloadCandidate&)>) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaOverload.cpp:11484:9
    #1 0xde4e7ca in clang::OverloadCandidateSet::NoteCandidates(std::__1::pair<clang::SourceLocation, clang::PartialDiagnostic>, clang::Sema&, clang::OverloadCandidateDisplayKind, llvm::ArrayRef<clang::Expr*>, llvm::StringRef, clang::SourceLocation, llvm::function_ref<bool (clang::OverloadCandidate&)>) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaOverload.cpp:11529:9
    #2 0xde6316f in FinishOverloadedCallExpr(clang::Sema&, clang::Scope*, clang::Expr*, clang::UnresolvedLookupExpr*, clang::SourceLocation, llvm::MutableArrayRef<clang::Expr*>, clang::SourceLocation, clang::Expr*, clang::OverloadCandidateSet*, clang::OverloadCandidate**, clang::OverloadingResult, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaOverload.cpp:12959:19
    #3 0xde6296b in clang::Sema::BuildOverloadedCallExpr(clang::Scope*, clang::Expr*, clang::UnresolvedLookupExpr*, clang::SourceLocation, llvm::MutableArrayRef<clang::Expr*>, clang::SourceLocation, clang::Expr*, bool, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaOverload.cpp:13032:10
    #4 0xd4e126b in clang::Sema::BuildCallExpr(clang::Scope*, clang::Expr*, clang::SourceLocation, llvm::MutableArrayRef<clang::Expr*>, clang::SourceLocation, clang::Expr*, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaExpr.cpp:6378:16
    #5 0xd53e073 in clang::Sema::ActOnCallExpr(clang::Scope*, clang::Expr*, clang::SourceLocation, llvm::MutableArrayRef<clang::Expr*>, clang::SourceLocation, clang::Expr*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaExpr.cpp:6275:7
    #6 0xcaa957f in clang::Parser::ParsePostfixExpressionSuffix(clang::ActionResult<clang::Expr*, true>) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExpr.cpp:2066:23
    #7 0xcaaf03e in clang::Parser::ParseCastExpression(clang::Parser::CastParseKind, bool, bool&, clang::Parser::TypeCastState, bool, bool*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExpr.cpp:1811:9
    #8 0xcaa5f3e in clang::Parser::ParseCastExpression(clang::Parser::CastParseKind, bool, clang::Parser::TypeCastState, bool, bool*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExpr.cpp:681:20
    #9 0xcaa15f3 in clang::Parser::ParseAssignmentExpression(clang::Parser::TypeCastState) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExpr.cpp:173:20
    #10 0xcaa13ad in clang::Parser::ParseExpression(clang::Parser::TypeCastState) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExpr.cpp:124:18
    #11 0xcbc8288 in clang::Parser::ParseExprStatement(clang::Parser::ParsedStmtContext) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseStmt.cpp:446:19
    #12 0xcbc2183 in clang::Parser::ParseStatementOrDeclarationAfterAttributes(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*, clang::Parser::ParsedAttributesWithRange&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseStmt.cpp:234:12
    #13 0xcbc119d in clang::Parser::ParseStatementOrDeclaration(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseStmt.cpp:106:20
    #14 0xcbdd830 in clang::Parser::ParseCompoundStatementBody(bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseStmt.cpp:1098:11
    #15 0xcbe0b67 in clang::Parser::ParseFunctionStatementBody(clang::Decl*, clang::Parser::ParseScope&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseStmt.cpp:2259:21
    #16 0xc9c26d4 in clang::Parser::ParseFunctionDefinition(clang::ParsingDeclarator&, clang::Parser::ParsedTemplateInfo const&, clang::Parser::LateParsedAttrList*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:1375:10
    #17 0xca17b5b in clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&, clang::DeclaratorContext, clang::SourceLocation*, clang::Parser::ForRangeInit*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseDecl.cpp:1924:27
    #18 0xc9bf6ee in clang::Parser::ParseDeclOrFunctionDefInternal(clang::Parser::ParsedAttributesWithRange&, clang::ParsingDeclSpec&, clang::AccessSpecifier) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:1135:10
    #19 0xc9bde93 in clang::Parser::ParseDeclarationOrFunctionDefinition(clang::Parser::ParsedAttributesWithRange&, clang::ParsingDeclSpec*, clang::AccessSpecifier) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:1151:12
    #20 0xc9bb56b in clang::Parser::ParseExternalDeclaration(clang::Parser::ParsedAttributesWithRange&, clang::ParsingDeclSpec*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:971:12
    #21 0xc9b5d18 in clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:716:12
    #22 0xc9a49cf in clang::ParseAST(clang::Sema&, bool, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseAST.cpp:158:20
    #23 0x8de7de0 in clang::FrontendAction::Execute() /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/FrontendAction.cpp:950:8
    #24 0x8ce176d in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:984:33
    #25 0x905ccb9 in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:278:25
    #26 0xb9bc9d in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/tools/driver/cc1_main.cpp:240:15
    #27 0xb93aa6 in ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/tools/driver/driver.cpp:330:12
    #28 0xb928ad in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/tools/driver/driver.cpp:407:12
    #29 0x7f3d1d0dc09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #30 0xb15209 in _start (/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang-12+0xb15209)

SUMMARY: MemorySanitizer: use-of-uninitialized-value /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaOverload.cpp:11484:9 in clang::OverloadCandidateSet::CompleteCandidates(clang::Sema&, clang::OverloadCandidateDisplayKind, llvm::ArrayRef<clang::Expr*>, clang::SourceLocation, llvm::function_ref<bool (clang::OverloadCandidate&)>)
Exiting

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  Clang :: SemaCUDA/deferred-oeverload.cu

In D87858#2280429, @jfb wrote:

Please provide documentation in this patch.

In D87791#2279885, @tra wrote:

In D87791#2279864, @yaxunl wrote:

It is requested by our debugger team, so it should work with amdgpu.

Is the naming scheme for GPU-side DWO files dictated by debugger? If that's the case, it may be worth adding a comment about that.

LGTM.

In D87791#2279821, @tra wrote:

In D87791#2278417, @yaxunl wrote:

Therefore in either case there is no need to rename the intermediate .o files since they are temporary files which have unique names.

The .dwo files are not temporary files. They are supposed to be shipped with .o files for debugging info.

Ack.
BTW, is split-dwarf useful for AMD GPUs on device side? I don't think we can currently utilize DWO files on device side with CUDA at all. To think of it, it's probably going to break GPU-side debugging as CUDA can only deal with dwarf info embedded in the GPU binary.
If it does not work for AMD GPUs, perhaps we should just disable it for GPUs.

In D84364#2279684, @hctim wrote:

Looks like this patch broke the MSan buildbots, PTAL (repro instructions https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild):

http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/46239/steps/check-clang%20msan/logs/stdio

FAIL: Clang :: SemaCUDA/deferred-oeverload.cu (11308 of 26387)
******************** TEST 'Clang :: SemaCUDA/deferred-oeverload.cu' FAILED ********************
Script:
--
: 'RUN: at line 1';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang -cc1 -internal-isystem /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/clang/12.0.0/include -nostdsysteminc -fcuda-is-device -fsyntax-only -verify=dev,com /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/test/SemaCUDA/deferred-oeverload.cu    -std=c++11 -fgpu-defer-diag
: 'RUN: at line 3';   /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang -cc1 -internal-isystem /b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/lib/clang/12.0.0/include -nostdsysteminc -fsyntax-only -verify=host,com /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/test/SemaCUDA/deferred-oeverload.cu    -std=c++11 -fgpu-defer-diag
--
Exit Code: 77

Command Output (stderr):
--
==41680==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0xddfbf73 in clang::OverloadCandidateSet::CompleteCandidates(clang::Sema&, clang::OverloadCandidateDisplayKind, llvm::ArrayRef<clang::Expr*>, clang::SourceLocation, llvm::function_ref<bool (clang::OverloadCandidate&)>) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaOverload.cpp:11484:9
    #1 0xde4e7ca in clang::OverloadCandidateSet::NoteCandidates(std::__1::pair<clang::SourceLocation, clang::PartialDiagnostic>, clang::Sema&, clang::OverloadCandidateDisplayKind, llvm::ArrayRef<clang::Expr*>, llvm::StringRef, clang::SourceLocation, llvm::function_ref<bool (clang::OverloadCandidate&)>) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaOverload.cpp:11529:9
    #2 0xde6316f in FinishOverloadedCallExpr(clang::Sema&, clang::Scope*, clang::Expr*, clang::UnresolvedLookupExpr*, clang::SourceLocation, llvm::MutableArrayRef<clang::Expr*>, clang::SourceLocation, clang::Expr*, clang::OverloadCandidateSet*, clang::OverloadCandidate**, clang::OverloadingResult, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaOverload.cpp:12959:19
    #3 0xde6296b in clang::Sema::BuildOverloadedCallExpr(clang::Scope*, clang::Expr*, clang::UnresolvedLookupExpr*, clang::SourceLocation, llvm::MutableArrayRef<clang::Expr*>, clang::SourceLocation, clang::Expr*, bool, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaOverload.cpp:13032:10
    #4 0xd4e126b in clang::Sema::BuildCallExpr(clang::Scope*, clang::Expr*, clang::SourceLocation, llvm::MutableArrayRef<clang::Expr*>, clang::SourceLocation, clang::Expr*, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaExpr.cpp:6378:16
    #5 0xd53e073 in clang::Sema::ActOnCallExpr(clang::Scope*, clang::Expr*, clang::SourceLocation, llvm::MutableArrayRef<clang::Expr*>, clang::SourceLocation, clang::Expr*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaExpr.cpp:6275:7
    #6 0xcaa957f in clang::Parser::ParsePostfixExpressionSuffix(clang::ActionResult<clang::Expr*, true>) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExpr.cpp:2066:23
    #7 0xcaaf03e in clang::Parser::ParseCastExpression(clang::Parser::CastParseKind, bool, bool&, clang::Parser::TypeCastState, bool, bool*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExpr.cpp:1811:9
    #8 0xcaa5f3e in clang::Parser::ParseCastExpression(clang::Parser::CastParseKind, bool, clang::Parser::TypeCastState, bool, bool*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExpr.cpp:681:20
    #9 0xcaa15f3 in clang::Parser::ParseAssignmentExpression(clang::Parser::TypeCastState) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExpr.cpp:173:20
    #10 0xcaa13ad in clang::Parser::ParseExpression(clang::Parser::TypeCastState) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseExpr.cpp:124:18
    #11 0xcbc8288 in clang::Parser::ParseExprStatement(clang::Parser::ParsedStmtContext) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseStmt.cpp:446:19
    #12 0xcbc2183 in clang::Parser::ParseStatementOrDeclarationAfterAttributes(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*, clang::Parser::ParsedAttributesWithRange&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseStmt.cpp:234:12
    #13 0xcbc119d in clang::Parser::ParseStatementOrDeclaration(llvm::SmallVector<clang::Stmt*, 32u>&, clang::Parser::ParsedStmtContext, clang::SourceLocation*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseStmt.cpp:106:20
    #14 0xcbdd830 in clang::Parser::ParseCompoundStatementBody(bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseStmt.cpp:1098:11
    #15 0xcbe0b67 in clang::Parser::ParseFunctionStatementBody(clang::Decl*, clang::Parser::ParseScope&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseStmt.cpp:2259:21
    #16 0xc9c26d4 in clang::Parser::ParseFunctionDefinition(clang::ParsingDeclarator&, clang::Parser::ParsedTemplateInfo const&, clang::Parser::LateParsedAttrList*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:1375:10
    #17 0xca17b5b in clang::Parser::ParseDeclGroup(clang::ParsingDeclSpec&, clang::DeclaratorContext, clang::SourceLocation*, clang::Parser::ForRangeInit*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseDecl.cpp:1924:27
    #18 0xc9bf6ee in clang::Parser::ParseDeclOrFunctionDefInternal(clang::Parser::ParsedAttributesWithRange&, clang::ParsingDeclSpec&, clang::AccessSpecifier) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:1135:10
    #19 0xc9bde93 in clang::Parser::ParseDeclarationOrFunctionDefinition(clang::Parser::ParsedAttributesWithRange&, clang::ParsingDeclSpec*, clang::AccessSpecifier) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:1151:12
    #20 0xc9bb56b in clang::Parser::ParseExternalDeclaration(clang::Parser::ParsedAttributesWithRange&, clang::ParsingDeclSpec*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:971:12
    #21 0xc9b5d18 in clang::Parser::ParseTopLevelDecl(clang::OpaquePtr<clang::DeclGroupRef>&, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/Parser.cpp:716:12
    #22 0xc9a49cf in clang::ParseAST(clang::Sema&, bool, bool) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Parse/ParseAST.cpp:158:20
    #23 0x8de7de0 in clang::FrontendAction::Execute() /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/FrontendAction.cpp:950:8
    #24 0x8ce176d in clang::CompilerInstance::ExecuteAction(clang::FrontendAction&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Frontend/CompilerInstance.cpp:984:33
    #25 0x905ccb9 in clang::ExecuteCompilerInvocation(clang::CompilerInstance*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/FrontendTool/ExecuteCompilerInvocation.cpp:278:25
    #26 0xb9bc9d in cc1_main(llvm::ArrayRef<char const*>, char const*, void*) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/tools/driver/cc1_main.cpp:240:15
    #27 0xb93aa6 in ExecuteCC1Tool(llvm::SmallVectorImpl<char const*>&) /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/tools/driver/driver.cpp:330:12
    #28 0xb928ad in main /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/tools/driver/driver.cpp:407:12
    #29 0x7f3d1d0dc09a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2409a)
    #30 0xb15209 in _start (/b/sanitizer-x86_64-linux-fast/build/llvm_build_msan/bin/clang-12+0xb15209)

SUMMARY: MemorySanitizer: use-of-uninitialized-value /b/sanitizer-x86_64-linux-fast/build/llvm-project/clang/lib/Sema/SemaOverload.cpp:11484:9 in clang::OverloadCandidateSet::CompleteCandidates(clang::Sema&, clang::OverloadCandidateDisplayKind, llvm::ArrayRef<clang::Expr*>, clang::SourceLocation, llvm::function_ref<bool (clang::OverloadCandidate&)>)
Exiting

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (1):
  Clang :: SemaCUDA/deferred-oeverload.cu

In D87791#2277887, @tra wrote:

Does this naming scheme the same as used for .o files? We may want to keep them in sync.

Other than that, LGTM.

Revised by Artem's comments. Extracted the common code of DiagnosticBuilder and PartialDiagnostic
and removed redundant code.

In D84362#2274884, @tra wrote:

In D84362#2274790, @yaxunl wrote:

There are use patterns expecting PartialDiagnosticInst << X << Y to continue to be a PartialDiagnostic&, e.g.

PartialDiagnosticAt PDAt(SourceLoc, PartialDiagnosticInst << X << Y);

However if we derive PartialDiagnostic and DiagnosticBuilder from a base class DiagnosticBuilderBase which implements the << operators, PartialDiagnosticInst << X << Y will become a DiagnosticBuilderBase&, then we can no longer write the above code.

That's one reason I use templates to implement << operators.

Do we want to sacrifice this convenience?

I don't think we have to.
AFAICT, virtually all such patterns (and there are only 20-ish of them in total) are used with EmitFormatDiagnostic(S.PDiag()) which could be adapted to accept DiagnosticBuilderBase and Sema::PDiag() changed to return PartialDiagnosticBuilder with no loss of convenience.

In D87321#2274731, @nemanjai wrote:

This broke the PPC LLD bot and the failure has been ignored for 4 days. I believe it should be fixed with 3bc3983f229.

In D84362#2274315, @aaron.ballman wrote:

In D84362#2271585, @tra wrote:

So, the idea here is to do some sort of duck-typing and allow DiagBuilder to work with both DiagnosticBuilder and PartialDiagnostic.

What bothers me is that unlike Diagnostic PartialDiagnostic seems to be commingling functionality of the builder with that of the diagnostic itself. I'm not sure if that's by design or just happened to be that way.

I think a better approach would be to refactor PartialDiagnostic and separate the builder functionality. That should make it possible to create a common diagnostic builder base class with Partial/Full diagnostic deriving their own builder, if needed.

That said, I'm not that familiar with the diags. Perhaps @rtrieu @aaron.ballman would have better ideas.

I'm similarly a bit uncomfortable with adding the SFINAE magic to make this work instead of making a base class that will work for either kind of diagnostic builder. I'm adding @rsmith to see if he has opinions as well.

fix tests

fix tests

revised by Fangrui's comments.

revised by Tony's and Konstantin's comments

need tests for --cuda-host-only and default host/device compilation, and tests for C/C++

ping. this is needed by https://reviews.llvm.org/D84364

In D84364#2255572, @tra wrote:

LGTM.

Nice!

To sum it up -- the patch introduces -fgpu-defer-diag flag which allows deferring overload resolution diagnostics, if overload set included candidates from both sides.

We may be deferring cases when we don't have to (e.g. df()->()callee2() should've errored out right away, even during host compilation, as there's no way it could ever be valid), but this approach is a good starting point. It affects only the interesting subset of diags, is not enabled by default, and can be refined further, if necessary.

In D84364#2201336, @tra wrote:

In D84364#2176091, @yaxunl wrote:

I added a Deferrable bit to the diagnostics which can be specified in td files. This can be added to individual diagnostic defs or added to a bunch of diagnostic defs all together.

This field is used to control whether a diagnostic message can be deferred.

This may be a case of "too much, but not enough". It will be unnecessary for most of the diagnostics we have. Overload resolution is likely to be the primary beneficiary, inline asm and exceptions may be two other classes, but I can't think of anything else ATM.
At the same time it may not be enough, because we also need to take into account where and when particular diagnostic is emitted. I.e. the same diagnostic may need to be postponed when we emit it from CUDA code, yet we may want to *not* postpone it if it's in the code which has nothing to do with CUDA. E.g. C++ code which has oveloading-related error in an inline function which would not be codegen'ed. I would expect such error to be reported as it would be if the same function was compiled in plain C++ mode.

Defer overload resolution diags only if there are wrong-sided candidates.

revised

revised by Tony's comments.

In D86376#2236704, @tra wrote:

In D86376#2234824, @tra wrote:

In D86376#2234719, @yaxunl wrote:

This patch appears to be somewhere in the gray area to me. My prior experience with CUDA suggests that it will make little to no difference. On the other hand, AMD GPUs may be different enough to prove me wrong. Without specific evidence, I still can't tell what's the case here.

Sorry, the overhead due to __hipPushConfigure/__hipPopConfigure is about 60 us. The typical kernel launching latency is about 500us, therefore the improvement is around 10%.

60 *micro seconds* to store/load something from memory? It does not sound right. 0.5 millisecond per kernel launch is also suspiciously high.
For CUDA it's ~5us (https://www.hpcs.cs.tsukuba.ac.jp/icpp2019/data/posters/Poster17-abst.pdf). If it does indeed take 60 microseconds to push/pop a O(cacheline) worth of launch config data, the implementation may be doing something wrong. We're talking about O(100) syscalls and that's way too much work for something that simple. What do those calls do?

Can you confirm that the units are indeed microseconds and not nanoseconds?

My previous measurements did not warming up, which caused some one time overhead due to device initialization and loading of device binary. With warm up, the call of __hipPushCallConfigure/__hipPopCallConfigure takes about 19 us. Based on the trace from rocprofile, the time spent inside these functions can be ignored. Most of the time is spent making the calls. These functions stay in a shared library, which may be the reason why they take such long time. Making them always_inline may get rid of the overhead, however, that would require exposing internal data structures.

In D86376#2234547, @tra wrote:

I'm OK with how the patch is implemented.
I'm still on the fence regarding whether it should be implemented.

In D86376#2234458, @yaxunl wrote:

`hipPushConfiguration/hipPopConfiguration' and kernel stub can cause 40 ns overhead, whereas we have requests to squeeze any overhead in kernel launching latency.

That's about the same as 1 cache miss. I'm willing to bet that it will be lost in the noise. Are there any real world benchmarks where it makes a difference?
Are those requests driven by a specific use case? Not all requests (even well intentioned ones) are worth implementing.
This patch appears to be somewhere in the gray area to me. My prior experience with CUDA suggests that it will make little to no difference. On the other hand, AMD GPUs may be different enough to prove me wrong. Without specific evidence, I still can't tell what's the case here.

In D86376#2234259, @tra wrote:

How much does this inlining buy you in practice? I.e. what's a typical launch latency before/after the patch? For CUDA, config push/pop is negligible compared to the cost of actually launching the kernel on the GPU. It is measurable if the launch is asynchronous, but queueing kernels fast, does not help all that much in the long run -- you eventually have to run those kernels on the GPU, so in most cases you're just spend a bit more time idling while waiting for the queued kernels to finish. To be beneficial, you'll need a finely balanced CPU/GPU workload and that's rather hard to achieve. Not to the point where the minor savings here would be meaningful. I would assume the situation on AMD GPUs is not that different.

revised by Tony's comments.

In D84822#2225968, @JonChesterfield wrote:

How does one opt out of this scheme?

OpenMP already has a convention for finding code objects in the host binary, used by amdgpu and other targets, which I don't think matches the above.

Where's the corresponding implementation? I think it's the llc part I need to read to understand whether this proposal can be used outside of HIP and amd's OpenCL.