This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Avoid using tuple where pair does suffice

Authored by maurossi on Jan 29 2023, 5:58 AM.



Fixes the following building errors, happening with official Android prebuilt clang 14 shipped with Android 13:

external/llvm-project/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:5491:13: error: no viable constructor or deduction g
uide for deduction of template arguments of 'tuple'

? std::tuple(HSAMD::V3::AssemblerDirectiveBegin,

external/llvm-project/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp:5493:13: error: no viable constructor or deduction g
uide for deduction of template arguments of 'tuple'

: std::tuple(HSAMD::AssemblerDirectiveBegin,

Fixes: 6443c0e ("[AMDGPU] Stop using make_pair and make_tuple. NFC.")

Diff Detail

Event Timeline

maurossi created this revision.Jan 29 2023, 5:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 29 2023, 5:58 AM
maurossi requested review of this revision.Jan 29 2023, 5:58 AM
arsenm accepted this revision.Jan 29 2023, 6:00 AM
This revision is now accepted and ready to land.Jan 29 2023, 6:00 AM


I have two questions

Since the issue happens also in release/16.x branch will D142839 be backported to release/16.x branch ?

Why has "x64 debian" build failed?

foad added a comment.Feb 6 2023, 1:42 AM

Why has "x64 debian" build failed?

Looks like the part that failed is checking that the lines you changed are formatted properly according to "clang-format". You can reproduce this locally by running .../llvm-project/clang/tools/clang-format/git-clang-format @^ and seeing if it updates any of the files you changed.

Hi Jay,
here is the output of 'git-clang-format --diff'

utente@utente-3TB:~/r-x86_kernel/external/llvm-project$ ./clang/tools/clang-format/git-clang-format --diff
diff --git a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
index c879b4914ee7..7ee1672049b3 100644
--- a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
+++ b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
@@ -5488,9 +5488,9 @@ bool AMDGPUAsmParser::ParseDirectiveHSAMetadata() {
   std::tie(AssemblerDirectiveBegin, AssemblerDirectiveEnd) =
           ? std::pair(HSAMD::V3::AssemblerDirectiveBegin,
-                       HSAMD::V3::AssemblerDirectiveEnd)
+                      HSAMD::V3::AssemblerDirectiveEnd)
           : std::pair(HSAMD::AssemblerDirectiveBegin,
-                       HSAMD::AssemblerDirectiveEnd);
+                      HSAMD::AssemblerDirectiveEnd);
   if (getSTI().getTargetTriple().getOS() != Triple::AMDHSA) {
     return Error(getLoc(),
maurossi updated this revision to Diff 495227.Feb 6 2023, 11:40 AM

Diff updated for conformance with git-clang-format

foad accepted this revision.Apr 11 2023, 3:49 AM

Patch LGTM, thanks. But I don't understand why the Android toolchain was complaining. Does it claim to support C++17?

Same issue happened on Android prebuilt clang 8 in NDK20. Seem it's having a stricter rule for deduction the template arguments.
Root cause should be the different definition of AssemblerDirectiveBegin/AssemblerDirectiveEnd.
It's defined as "const char * ......"in std::tie which is the left value.
But the right value which in HSAMD::V3 and HSAMD:: it's defined as "constexpr char ......[]".
Tried to make it defined as "const char * ......" in HSAMD::V3 and HSAMD in local, it can pass for this error(just a test I don't think it's the right fix).

foad added a comment.Apr 20 2023, 6:29 AM

@maurossi are you planning to commit this? Would you like me to commit it for you if you don't have access?

This revision was automatically updated to reflect the committed changes.

Hello Jay,

sorry for late response,
being an external contributor I don't think I should have the rights to
apply commits in the official public llvm-project code base

Moreover in this moment I cannot even login via my google account,
thanks for having applied the patch