[mlir][sparse] Restore case coverage warning fix

[NathanielMcVicar]

Summary

This restores the fix from D134925 to make MSVC and clang happy.

[stella.stamenova]

LGTM, thanks for fixing the build breaks!

[aartbik]

mlir/lib/ExecutionEngine/SparseTensor/File.cpp

27

this needs a bazel fix, coming up....

[aartbik]

mlir/lib/ExecutionEngine/SparseTensor/File.cpp

112

Note that we have our own macro mechanism for this, which we would have preferred over pulling in llvm support for just this macro.
But we will fix that going forward....

[NathanielMcVicar]

mlir/lib/ExecutionEngine/SparseTensor/File.cpp

112

For reasons that I don't understand MSVC doesn't recognize MLIR_SPARSETENSOR_FATAL as returning a value here, despite exit(). Sorry for making more work re bazel!

[wrengr]

mlir/lib/ExecutionEngine/SparseTensor/File.cpp

112

Also note that the warnings here were already fixed by D134925, which @stella.stamenova signed off on. Honestly I don't see how this change could've had the diff apply cleanly given that D134925 already landed

[NathanielMcVicar]

mlir/lib/ExecutionEngine/SparseTensor/File.cpp

112

The fix in D134925 broke clang (for different reasons) and was reverted. I should have included that in the summary.
https://github.com/llvm/llvm-project/commit/e898be2f6edbb886af2f6b23e2f5db5210535620

[wrengr]

mlir/lib/ExecutionEngine/SparseTensor/File.cpp

112

Which version of MSVC are you using? Because the version using MLIR_SPARSETENSOR_FATAL works fine on our end.

Fwiw, the trick that makes llvm_unreachable work is that it unfolds to a function marked with the [[noreturn]] attribute. The exit() function should also be marked [[noreturn]] as well though. So if I had to guess, I'm thinking your version of MSVC can't see that the do { ... } while (0) wrapping cannot change the [[noreturn]]-ness of the exit called at the end of that trivial loop.

[stella.stamenova]

As Nate said, https://github.com/llvm/llvm-project/commit/e898be2f6edbb886af2f6b23e2f5db5210535620 broke the windows mlir bot which then stayed red for a couple of days. We wanted to make sure to get it back to green and this change seemed the most appropriate. We are not particularly tied to this change or another change but simply reverting this will get us back to a red state. @wrengr @aartbik if you have a better way to address the issue, that would be great, but we do not want the bot to be red again.

You can see the bot information and builds here: https://lab.llvm.org/buildbot/#/builders/13

These are the versions it is using:

Windows Server 2016 Version 1607
cmake version 3.22.0
Microsoft (R) C/C++ Optimizing Compiler Version 19.29.30137
git version 2.34.1.windows.1
Microsoft Visual Studio Community 2019 Version 16.11.7
Ninja version 1.10.2
Python version 3.9.9

These should all fall within the supported LLVM versions of the tools.

[NathanielMcVicar]

mlir/lib/ExecutionEngine/SparseTensor/File.cpp

112

It seems odd that any version of MSVC would miss it, but MLIR_SPARSETENSOR_FATAL doesn't produce the warning outside the switch on our internal CI, so it would likely be ok on the buildbot as well. I'm happy to create a new revision to fix it, or you can clean it up in the next change as suggested above to minimize churn. Let me know, and sorry again for the confusion.

[vitalybuka]

This is probably pre-existing issue exposed by this patch, but something needs to be done to fix the bot

FAIL: MLIR :: mlir-cpu-runner/async.mlir (65775 of 68681)
******************** TEST 'MLIR :: mlir-cpu-runner/async.mlir' FAILED ********************
Script:
--
: 'RUN: at line 1';     /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/mlir-opt /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/mlir/test/mlir-cpu-runner/async.mlir -pass-pipeline="async-to-async-runtime,func.func(async-runtime-ref-counting,async-runtime-ref-counting-opt),convert-async-to-llvm,func.func(convert-linalg-to-loops,convert-scf-to-cf),convert-linalg-to-llvm,convert-memref-to-llvm,func.func(convert-arith-to-llvm),convert-func-to-llvm,reconcile-unrealized-casts"  | /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/mlir-cpu-runner                                                           -e main -entry-point-result=void -O0                                    -shared-libs=/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/./lib/libmlir_c_runner_utils.so       -shared-libs=/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/./lib/libmlir_runner_utils.so         -shared-libs=/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/./lib/libmlir_async_runtime.so    | /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/FileCheck /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/mlir/test/mlir-cpu-runner/async.mlir
--
Exit Code: 2
Command Output (stderr):
--
=================================================================
==3101281==ERROR: AddressSanitizer: odr-violation (0x7f37935c6f40):
  [1] size=4 'llvm::EnableABIBreakingChecks' /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/ABIBreak.cpp
  [2] size=4 'llvm::EnableABIBreakingChecks' /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/ABIBreak.cpp
These globals were registered at these points:
  [1]:
    #0 0x56152a12ea07 in __asan_register_globals /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:356:3
    #1 0x7f37a2a0247d  (/lib64/ld-linux-x86-64.so.2+0x647d) (BuildId: 61ef896a699bb1c2e4e231642b2e1688b2f1a61e)
  [2]:
    #0 0x56152a12ea07 in __asan_register_globals /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:356:3
    #1 0x7f37a2a0247d  (/lib64/ld-linux-x86-64.so.2+0x647d) (BuildId: 61ef896a699bb1c2e4e231642b2e1688b2f1a61e)
==3101281==HINT: if you don't care about these errors you may set ASAN_OPTIONS=detect_odr_violation=0
SUMMARY: AddressSanitizer: odr-violation: global 'llvm::EnableABIBreakingChecks' at /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/ABIBreak.cpp
==3101281==ABORTING
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/FileCheck /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/mlir/test/mlir-cpu-runner/async.mlir
--
********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90
FAIL: MLIR :: mlir-cpu-runner/async-value.mlir (65779 of 68681)
******************** TEST 'MLIR :: mlir-cpu-runner/async-value.mlir' FAILED ********************
Script:
--
: 'RUN: at line 1';     /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/mlir-opt /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/mlir/test/mlir-cpu-runner/async-value.mlir -pass-pipeline="async-to-async-runtime,func.func(async-runtime-ref-counting,async-runtime-ref-counting-opt),convert-async-to-llvm,func.func(convert-arith-to-llvm),convert-vector-to-llvm,convert-memref-to-llvm,convert-func-to-llvm,reconcile-unrealized-casts"  | /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/mlir-cpu-runner                                                           -e main -entry-point-result=void -O0                                    -shared-libs=/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/./lib/libmlir_c_runner_utils.so       -shared-libs=/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/./lib/libmlir_runner_utils.so         -shared-libs=/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/./lib/libmlir_async_runtime.so    | /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/FileCheck /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/mlir/test/mlir-cpu-runner/async-value.mlir --dump-input=always
--
Exit Code: 2
Command Output (stderr):
--
=================================================================
==3101291==ERROR: AddressSanitizer: odr-violation (0x7f546321af40):
  [1] size=4 'llvm::EnableABIBreakingChecks' /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/ABIBreak.cpp
  [2] size=4 'llvm::EnableABIBreakingChecks' /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/ABIBreak.cpp
These globals were registered at these points:
  [1]:
    #0 0x558346dcaa07 in __asan_register_globals /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:356:3
    #1 0x7f54879a447d  (/lib64/ld-linux-x86-64.so.2+0x647d) (BuildId: 61ef896a699bb1c2e4e231642b2e1688b2f1a61e)
  [2]:
    #0 0x558346dcaa07 in __asan_register_globals /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:356:3
    #1 0x7f54879a447d  (/lib64/ld-linux-x86-64.so.2+0x647d) (BuildId: 61ef896a699bb1c2e4e231642b2e1688b2f1a61e)
==3101291==HINT: if you don't care about these errors you may set ASAN_OPTIONS=detect_odr_violation=0
SUMMARY: AddressSanitizer: odr-violation: global 'llvm::EnableABIBreakingChecks' at /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/ABIBreak.cpp
==3101291==ABORTING
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/FileCheck /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/mlir/test/mlir-cpu-runner/async-value.mlir --dump-input=always
--
********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90
FAIL: MLIR :: mlir-cpu-runner/async-error.mlir (65818 of 68681)
******************** TEST 'MLIR :: mlir-cpu-runner/async-error.mlir' FAILED ********************
Script:
--
: 'RUN: at line 1';     /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/mlir-opt /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/mlir/test/mlir-cpu-runner/async-error.mlir -pass-pipeline="async-to-async-runtime,func.func(async-runtime-ref-counting,async-runtime-ref-counting-opt),convert-async-to-llvm,func.func(convert-linalg-to-loops,convert-scf-to-cf),convert-linalg-to-llvm,convert-vector-to-llvm,func.func(convert-arith-to-llvm),convert-func-to-llvm,reconcile-unrealized-casts"  | /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/mlir-cpu-runner                                                           -e main -entry-point-result=void -O0                                    -shared-libs=/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/./lib/libmlir_c_runner_utils.so       -shared-libs=/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/./lib/libmlir_runner_utils.so         -shared-libs=/b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/./lib/libmlir_async_runtime.so    | /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/FileCheck /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/mlir/test/mlir-cpu-runner/async-error.mlir --dump-input=always
--
Exit Code: 2
Command Output (stderr):
--
=================================================================
==3101265==ERROR: AddressSanitizer: odr-violation (0x7f6ced60cf40):
  [1] size=4 'llvm::EnableABIBreakingChecks' /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/ABIBreak.cpp
  [2] size=4 'llvm::EnableABIBreakingChecks' /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/ABIBreak.cpp
These globals were registered at these points:
  [1]:
    #0 0x559ba6f8ba07 in __asan_register_globals /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:356:3
    #1 0x7f6d0d64447d  (/lib64/ld-linux-x86-64.so.2+0x647d) (BuildId: 61ef896a699bb1c2e4e231642b2e1688b2f1a61e)
  [2]:
    #0 0x559ba6f8ba07 in __asan_register_globals /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/compiler-rt/lib/asan/asan_globals.cpp:356:3
    #1 0x7f6d0d64447d  (/lib64/ld-linux-x86-64.so.2+0x647d) (BuildId: 61ef896a699bb1c2e4e231642b2e1688b2f1a61e)
==3101265==HINT: if you don't care about these errors you may set ASAN_OPTIONS=detect_odr_violation=0
SUMMARY: AddressSanitizer: odr-violation: global 'llvm::EnableABIBreakingChecks' at /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/llvm/lib/Support/ABIBreak.cpp
==3101265==ABORTING
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm_build_asan/bin/FileCheck /b/sanitizer-x86_64-linux-bootstrap-asan/build/llvm-project/mlir/test/mlir-cpu-runner/async-error.mlir --dump-input=always
--

[wrengr]

Aha, if D134925 got reverted then that explains how the diff applies :)

Sorry I've been a bit scattered in my comments. I've been dealing with some personal stuff the past couple weeks so have been in and out sporadically, and a crisis flared up just after this differential came to my attention.

I can check on my end, but I wonder if simply using MLIR_SPARSETENSOR_FATAL after the switch rather than as the default case would suffice to make both MSVC and Clang happy. If so, then I'd be perfectly happy signing off on that.

[stella.stamenova]

We ended up with https://reviews.llvm.org/D135304 as the currently committed change. I believe this worked correctly everywhere, but if anyone knows of a break, we should address that as well.

[wrengr]

I just saw D135304 landed with the change I suggested. With that in place, is this differential still necessary? i.e., is LLVM's Windows buildbot still red?

[wrengr]

Hmm, git-blame is showing that this differential has landed in github yet phabricator indicates it's still merely "accepted". I'm not too familiar with how phabricator interacts with github, but should this differential be closed?

[stella.stamenova]

In D135126#3840989, @wrengr wrote:

Hmm, git-blame is showing that this differential has landed in github yet phabricator indicates it's still merely "accepted". I'm not too familiar with how phabricator interacts with github, but should this differential be closed?

This was committed but then re-opened and reverted. We don't need it any more.

mlir/lib/ExecutionEngine/SparseTensor/File.cpp

 // tensor manipulations.  The functionality of the support library is meant
 // to simplify benchmarking, testing, and debugging MLIR code operating on
 // sparse tensors.  However, the provided functionality is **not** part of
 // core MLIR itself.
 //
 //===----------------------------------------------------------------------===//
 
 #include "mlir/ExecutionEngine/SparseTensor/File.h"
+#include "llvm/Support/ErrorHandling.h"
 
 #include <cctype>
 #include <cstring>
 
 using namespace mlir::sparse_tensor;
 
 /// Opens the file for reading.
 void SparseTensorFile::openFile() {
case ValueKind::kComplex:
     // require a complex primary-type.
     return isComplexPrimaryType(valTy);
   case ValueKind::kUndefined:
     // The "extended" FROSTT format doesn't specify a ValueKind.
     // So we allow implicitly converting the stored values to both
     // integer and floating primary-types.
     return isRealPrimaryType(valTy);
   }
+  llvm_unreachable("unknown ValueKind");
 }
 
 /// Helper to convert C-style strings (i.e., '\0' terminated) to lower case.
 static inline void toLower(char *token) {
   for (char *c = token; *c; ++c)
     *c = tolower(*c);
 }