[ADT] Work around MSVC bug affecting `get(enumerator_result)`

[kuhar]

Summary

This happened on a small number of MSVC releases (19.31.31xxx, Visual Studio 2022 17.1.x), and worked fine on everything else.

The issue seemed to be related to return type deduction on a function with and if constexpr; the compiler got confused and deduced different function return type from the type of the return statement.

The workaround is to split get into two functions using enable_if.

[dstuttard]

Confirmed that this fixes the issue I was seeing for MSVC 17.1.

[jurahul]

Looks like there are still issues with MSVC 14.29.xxxx

C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\utility(492): error C2338: tuple index out of bounds
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\utility(504): note: see reference to class template instantiation 'std::tuple_element<0,std::tuple<>>' being compiled
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\utility(504): note: see reference to class template instantiation 'std::tuple_element<1,std::tuple<const mlir::utils::IteratorType &>>' being compiled
external/llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp(1152): note: see reference to class template instantiation 'std::tuple_element<2,std::tuple<size_t,const mlir::utils::IteratorType &>>' being compiled
external/llvm-project/llvm/include\llvm/ADT/STLExtras.h(2323): note: see reference to alias template instantiation 'std::tuple_element_t<2,std::tuple<size_t,const mlir::utils::IteratorType &>>' being compiled

[kuhar]

In D146893#4227583, @jurahul wrote:

Looks like there are still issues with MSVC 14.29.xxxx

C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\utility(492): error C2338: tuple index out of bounds
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\utility(504): note: see reference to class template instantiation 'std::tuple_element<0,std::tuple<>>' being compiled
C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\VC\Tools\MSVC\14.29.30133\include\utility(504): note: see reference to class template instantiation 'std::tuple_element<1,std::tuple<const mlir::utils::IteratorType &>>' being compiled
external/llvm-project/mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp(1152): note: see reference to class template instantiation 'std::tuple_element<2,std::tuple<size_t,const mlir::utils::IteratorType &>>' being compiled
external/llvm-project/llvm/include\llvm/ADT/STLExtras.h(2323): note: see reference to alias template instantiation 'std::tuple_element_t<2,std::tuple<size_t,const mlir::utils::IteratorType &>>' being compiled

Gah... Are you able to check if replacing std::tuple_element_t<I, value_reference_tuple> with decltype(auto) fixes that? The explicit return type was not necessary on 2022 17.1 but something I used to debug the previous failure and never bothered undoing.

[jmorse]

Note that I'm seeing this too with Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29915 for x64.

[kuhar]

In D146893#4227652, @jmorse wrote:

Note that I'm seeing this too with Microsoft (R) C/C++ Optimizing Compiler Version 19.28.29915 for x64.

It'd be great if we had buildbots that cover a wider range of MSVC versions...

Gah... Are you able to check if replacing std::tuple_element_t<I, value_reference_tuple> with decltype(auto) fixes that? The explicit return type was not necessary on 2022 17.1 but something I used to debug the previous failure and never bothered undoing.

Either this or moving enable_if to the return type should fix this, I would guess. Could either of you, @jmorse or @jurahul, try that out? This would save me a couple of hours of work as I don't have these versions available to me.

[probinson]

llvm/include/llvm/ADT/STLExtras.h

2323

Tuple index should be decremented, yes?

[kuhar]

llvm/include/llvm/ADT/STLExtras.h

2323

No, it's should be as-is. value_reference_tuple includes index as the first element, which is not the case for Storage.

Alternatively, we could have std::tuple_element_t<I - 1, range_reference_tuple>, but that shouldn't change anything.

[probinson]

It'd be great if we had buildbots that cover a wider range of MSVC versions...

I agree, but my colleagues who pay more attention to this stuff claim there's only one bot actually using MSVC at all (the llvm-clang-x86_64-sie-win bot). They tell me all the other public Windows bots are using clang-cl, apparently, which doesn't help those of us downstream with big MSVC farms.

[jurahul]

I have pushed a fix the build failure with https://github.com/llvm/llvm-project/commit/b337735390659a7aa79bcefd1bebb89d7f278194

llvm/include/llvm/ADT/STLExtras.h

template <typename... Refs> struct enumerator_result<std::size_t, Refs...> {
   /// index.
   decltype(auto) value() const {
     if constexpr (NumRefs == 1)
       return std::get<0>(Storage);
     else
       return Storage;
   }
 
-  /// Returns the value at index `I`. This includes the index.
-  template <std::size_t I>
-  friend decltype(auto) get(const enumerator_result &Result) {
-    static_assert(I < NumValues, "Index out of bounds");
-    if constexpr (I == 0)
-      return Result.Idx;
-    else
-      return std::get<I - 1>(Result.Storage);
+  /// Returns the value at index `I`. This case covers the index.
+  template <std::size_t I, typename = std::enable_if_t<I == 0>>
+  friend std::size_t get(const enumerator_result &Result) {
+  }
+
+  /// Returns the value at index `I`. This case covers references to the
+  /// iteratees.
+  template <std::size_t I, typename = std::enable_if_t<I != 0>>
+  friend std::tuple_element_t<I, value_reference_tuple>
+  get(const enumerator_result &Result) {
+    // Note: This is a separate function from the other `get`, instead of an
+    // `if constexpr` case, to work around an MSVC 19.31.31XXX compiler
+    // (Visual Studio 2022 17.1) return type deduction bug.
   }
 
   template <typename... Ts>
   friend bool operator==(const enumerator_result &Result,
                          const std::tuple<std::size_t, Ts...> &Other) {
     static_assert(NumRefs == sizeof...(Ts), "Size mismatch");
     if (Result.Idx != std::get<0>(Other))
       return false;