[IR] Simplify HasCachedHash with is_detected (NFC) #159510

kazutakahirata · 2025-09-18T06:01:06Z

With is_detected, we don't need to implement a SFINAE trick on our own.

llvmbot · 2025-09-18T06:01:40Z

@llvm/pr-subscribers-llvm-ir

Author: Kazu Hirata (kazutakahirata)

Changes

With is_detected, we don't need to implement a SFINAE trick on our own.

Full diff: https://github.com/llvm/llvm-project/pull/159510.diff

1 Files Affected:

(modified) llvm/lib/IR/Metadata.cpp (+2-7)

diff --git a/llvm/lib/IR/Metadata.cpp b/llvm/lib/IR/Metadata.cpp
index fc78a5b299f49..09e25ceaf59c3 100644
--- a/llvm/lib/IR/Metadata.cpp
+++ b/llvm/lib/IR/Metadata.cpp
@@ -986,15 +986,10 @@ static T *uniquifyImpl(T *N, DenseSet<T *, InfoT> &Store) {
 }
 
 template <class NodeTy> struct MDNode::HasCachedHash {
-  using Yes = char[1];
-  using No = char[2];
-  template <class U, U Val> struct SFINAE {};
-
   template <class U>
-  static Yes &check(SFINAE<void (U::*)(unsigned), &U::setHash> *);
-  template <class U> static No &check(...);
+  using check = decltype(static_cast<void (U::*)(unsigned)>(&U::setHash));
 
-  static const bool value = sizeof(check<NodeTy>(nullptr)) == sizeof(Yes);
+  static constexpr bool value = is_detected<check, NodeTy>::value;
 };
 
 MDNode *MDNode::uniquify() {

mikaelholmen · 2025-09-18T18:18:18Z

Hi,

I have no idea what is happening but I noticed that if we build this patch with clang (wer're using 18.1) and run lit tests then the following tests fail:

Failed Tests (16):
  LLVM :: Analysis/TypeBasedAliasAnalysis/dse.ll
  LLVM :: CodeGen/AArch64/merge-scoped-aa-store.ll
  LLVM :: DebugInfo/macro_link.ll
  LLVM :: Linker/unique-fwd-decl-a.ll
  LLVM :: Linker/unique-fwd-decl-order.ll
  LLVM :: Transforms/LoadStoreVectorizer/AMDGPU/aa-metadata.ll
  LLVM :: Transforms/SLPVectorizer/X86/gathered-shuffle-resized.ll
  LLVM :: Verifier/noalias_scope_decl.ll
  LLVM-Unit :: IR/./IRTests/GenericDINodeTest/get
  LLVM-Unit :: IR/./IRTests/IRBuilderTest/DIBuilderMacro
  LLVM-Unit :: IR/./IRTests/MDNodeTest/DistinctOnUniquingCollision
  LLVM-Unit :: IR/./IRTests/MDNodeTest/NullOperand
  LLVM-Unit :: IR/./IRTests/MDNodeTest/UniquedOnDeletedOperand
  LLVM-Unit :: IR/./IRTests/MDNodeTest/handleChangedOperandRecursion
  LLVM-Unit :: IR/./IRTests/MDNodeTest/replaceWithPermanent
  LLVM-Unit :: IR/./IRTests/MDNodeTest/replaceWithUniquedChangedOperand

One example of a failure:

 FAIL: LLVM-Unit :: IR/./IRTests/3/26 (82881 of 87022)
 ******************** TEST 'LLVM-Unit :: IR/./IRTests/3/26' FAILED ********************
 Script(shard):
 --
 GTEST_OUTPUT=json:/repo/llvm-main-clang/build-all-builtins/unittests/IR/./IRTests-LLVM-Unit-28624-3-26.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=26 GTEST_SHARD_INDEX=3 /repo/llvm-main-clang/build-all-builtins/unittests/IR/./IRTests
 --

 Script:
 --
 /repo/llvm-main-clang/build-all-builtins/unittests/IR/./IRTests --gtest_filter=MDNodeTest.UniquedOnDeletedOperand
 --
 ../llvm-project/llvm/unittests/IR/MetadataTest.cpp:555: Failure
 Expected equality of these values:
   N
     Which is: 0x556bd162d658
   MDTuple::get(Context, NullOps)
     Which is: 0x556bd162d688


 ../llvm-project/llvm/unittests/IR/MetadataTest.cpp:555
 Expected equality of these values:
   N
     Which is: 0x556bd162d658
   MDTuple::get(Context, NullOps)
     Which is: 0x556bd162d688

If we build with gcc (13.3) everything seem to work.

So something funny seems to happen, at least when using clang 18.1.

Does anyone else see this?

justinfargnoli · 2025-09-18T18:29:59Z

@mikaelholmen, I'm also seeing the following tests fail locally as a result of this commit:

Failed Tests (13):
  LLVM :: Analysis/TypeBasedAliasAnalysis/dse.ll
  LLVM :: DebugInfo/macro_link.ll
  LLVM :: Linker/unique-fwd-decl-a.ll
  LLVM :: Linker/unique-fwd-decl-order.ll
  LLVM :: Verifier/noalias_scope_decl.ll
  LLVM-Unit :: IR/./IRTests/GenericDINodeTest/get
  LLVM-Unit :: IR/./IRTests/IRBuilderTest/DIBuilderMacro
  LLVM-Unit :: IR/./IRTests/MDNodeTest/DistinctOnUniquingCollision
  LLVM-Unit :: IR/./IRTests/MDNodeTest/NullOperand
  LLVM-Unit :: IR/./IRTests/MDNodeTest/UniquedOnDeletedOperand
  LLVM-Unit :: IR/./IRTests/MDNodeTest/handleChangedOperandRecursion
  LLVM-Unit :: IR/./IRTests/MDNodeTest/replaceWithPermanent
  LLVM-Unit :: IR/./IRTests/MDNodeTest/replaceWithUniquedChangedOperand


Testing Time: 63.54s

Total Discovered Tests: 73695
  Skipped          :   649 (0.88%)
  Unsupported      : 48670 (66.04%)
  Passed           : 24264 (32.92%)
  Expectedly Failed:    99 (0.13%)
  Failed           :    13 (0.02%)

However, I haven't root caused this. (i.e. perhaps these failures can be attributed to a local change)

For context, here's a subset of my build configuration:

    'cmake',
    '-G', 'Ninja',
    '-DCMAKE_BUILD_TYPE=Debug',
    '-DLLVM_ENABLE_ASSERTIONS=ON',
    '-Wno-dev',
    '-DLLVM_CCACHE_BUILD=ON',
    '-DLLVM_OPTIMIZED_TABLEGEN=ON',
    '-DLLVM_ENABLE_LLD=ON',
    '-DCMAKE_C_COMPILER=/usr/bin/clang-18',
    '-DCMAKE_CXX_COMPILER=/usr/bin/clang++-18',
    '-DBUILD_SHARED_LIBS=ON',
    '-DCMAKE_CUDA_COMPILER=/usr/local/cuda/bin/nvcc',

I have not tried building with gcc or any other C++ compiler.

rupprecht · 2025-09-18T18:30:49Z

Does anyone else see this?

Also see it on the bazel buildbots, which bisects to this change: https://buildkite.com/llvm-project/upstream-bazel/builds/151045

…#159622) This reverts commit d6b7ac8. Build breakages reported on the PR hint at not working with certain versions of the host compiler.

kazutakahirata · 2025-09-18T22:09:17Z

I've reproduced the failure with clang version 18.1.8.

I ran the two versions side by side:

template <class NodeTy> struct MDNode::HasCachedHash {
  using Yes = char[1];
  using No = char[2];
  template <class U, U Val> struct SFINAE {};

  template <class U>
  static Yes &check(SFINAE<void (U::*)(unsigned), &U::setHash> *);
  template <class U> static No &check(...);

  static const bool value = sizeof(check<NodeTy>(nullptr)) == sizeof(Yes);
};

template <class NodeTy> struct MDNode::HasCachedHashNew {
  template <class U>
  using check = decltype(static_cast<void (U::*)(unsigned)>(&U::setHash));

  static constexpr bool value = is_detected<check, NodeTy>::value;
};
:
:
:
    static_assert(HasCachedHash<CLASS>::value ==                               \
                  HasCachedHashNew<CLASS>::value);                             \

The static_assert doesn't trigger with clang version 19.1.1, but clang version 18.1.8 somehow thinks:

HasCachedHash<GenericDINode>::value == true
HasCachedHashNew<GenericDINode>::value == false

even though GenericDINode does have the setHash method:

 void setHash(unsigned Hash) { SubclassData32 = Hash; }

kuhar · 2025-09-19T00:42:39Z

using check = decltype(static_cast<void (U::*)(unsigned)>(&U::setHash));

This check is weird -- I'd expect something like std::declval<U>().setHash(0u) instead of trying to check if a static cast succeeds.

This patch modernizes HasCachedHash. - "struct SFINAE" is replaced with identically defined SameType. - The return types Yes and No are replaced with std::true_type and std::false_type. My previous attempt (llvm#159510) to clean up HasCachedHash failed on clang++-18, but this version works with clang++-18.

kazutakahirata · 2025-09-20T05:31:53Z

using check = decltype(static_cast<void (U::*)(unsigned)>(&U::setHash));

This check is weird -- I'd expect something like std::declval<U>().setHash(0u) instead of trying to check if a static cast succeeds.

@kuhar I don't mean to dig too deep into this, but I played with a reduced testcase of Metadata.cpp. It looks like the behavior of clang++-18 depends on whether setHash is private or not. Anyway, I just posted a new version #159902.

This patch modernizes HasCachedHash. - "struct SFINAE" is replaced with identically defined SameType. - The return types Yes and No are replaced with std::true_type and std::false_type. My previous attempt (#159510) to clean up HasCachedHash failed on clang++-18, but this version works with clang++-18.

[IR] Simplify HasCachedHash with is_detected (NFC)

9cae2d4

With is_detected, we don't need to implement a SFINAE trick on our own.

kazutakahirata requested review from arsenm, shiltian, tgymnich and kuhar September 18, 2025 06:01

llvmbot added the llvm:ir label Sep 18, 2025

arsenm approved these changes Sep 18, 2025

View reviewed changes

kuhar approved these changes Sep 18, 2025

View reviewed changes

kazutakahirata merged commit d6b7ac8 into llvm:main Sep 18, 2025
11 checks passed

kazutakahirata deleted the cleanup_20250917_IR_is_detected branch September 18, 2025 15:56

kazutakahirata mentioned this pull request Sep 20, 2025

[IR] Modernize HasCachedHash (NFC) #159902

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[IR] Simplify HasCachedHash with is_detected (NFC) #159510

[IR] Simplify HasCachedHash with is_detected (NFC) #159510

kazutakahirata commented Sep 18, 2025

Uh oh!

llvmbot commented Sep 18, 2025

Uh oh!

Uh oh!

mikaelholmen commented Sep 18, 2025

Uh oh!

justinfargnoli commented Sep 18, 2025 •

edited

Loading

Uh oh!

rupprecht commented Sep 18, 2025

Uh oh!

kazutakahirata commented Sep 18, 2025

Uh oh!

kuhar commented Sep 19, 2025

Uh oh!

kazutakahirata commented Sep 20, 2025

Uh oh!

Uh oh!

[IR] Simplify HasCachedHash with is_detected (NFC) #159510

[IR] Simplify HasCachedHash with is_detected (NFC) #159510

Conversation

kazutakahirata commented Sep 18, 2025

Uh oh!

llvmbot commented Sep 18, 2025

Uh oh!

Uh oh!

mikaelholmen commented Sep 18, 2025

Uh oh!

justinfargnoli commented Sep 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rupprecht commented Sep 18, 2025

Uh oh!

kazutakahirata commented Sep 18, 2025

Uh oh!

kuhar commented Sep 19, 2025

Uh oh!

kazutakahirata commented Sep 20, 2025

Uh oh!

Uh oh!

justinfargnoli commented Sep 18, 2025 •

edited

Loading