Build warning spew

Question

Build warning spew

silvasean opened this issue 2 years ago · 16 comments

Hi, MHLO has a number of build warnings that are causing spew in the Torch-MLIR build.

 /main_checkout/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc: In function ‘mlir::LogicalResult mlir::mhlo::sortOpInferDefaultDimension(mlir::mhlo::SortOp, mlir::PatternRewriter&)’:
  /main_checkout/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:6593:22: warning: comparison of integer expressions of different signedness: ‘uint64_t’ {aka ‘long unsigned int’} and ‘int’ [-Wsign-compare]
   6593 |   if (op.dimension() != -1) {

  /main_checkout/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc: In member function ‘mlir::LogicalResult mlir::mhlo::ReduceWindowOp::verify()’:
  /main_checkout/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:4210:29: warning: comparison of integer expressions of different signedness: ‘int64_t’ {aka ‘long int’} and ‘size_t’ {aka ‘long unsigned int’} [-Wsign-compare]
   4210 |     if (inputType.getRank() != windowDims.size())
        |         ~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~

  /main_checkout/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:6011:26: warning: extra ‘;’ [-Wpedantic]
   6011 | BINARY_FOLDER(MinOp, Min);
        |                          ^

For full list search for "mhlo" in https://github.com/llvm/torch-mlir/runs/7598076835?check_suite_focus=true

Answer 1 · 2022-08-01T21:24:11.000Z

Can you provide an update after a366977 ?

Answer 2 · 2022-08-17T00:57:20.000Z

I still am seeing:

/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:6450:33: warning: extra ';' outside of a function is incompatible with C++98 [-Wc++98-compat-extra-semi]
UNARY_FOLDER(NegOp, std::negate);

/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:1470:21: warning: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Wsign-compare]
  for (int i = 0; i < startIndexMap.size(); ++i)

In file included from /usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:167:
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/build/tools/torch-mlir/mlir-hlo/lib/Dialect/mhlo/IR/mhlo_canonicalize.inc:10:30: warning: unused function '__mlir_ods_local_type_constraint_mhlo_canonicalize0' [-Wunused-function]
static ::mlir::LogicalResult __mlir_ods_local_type_constraint_mhlo_canonicalize0(

Answer 3 · 2022-09-07T00:36:40.000Z

Currently seeing these:

/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:828:21: warning: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Wsign-compare]
  for (int i = 0; i < getOperands().size(); ++i) {
                  ~ ^ ~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:1180:42: warning: comparison of integers of different signs: 'int64_t' (aka 'long') and 'size_t' (aka 'unsigned long') [-Wsign-compare]
    if (operandIndex < 0 || operandIndex >= operands().size())
                            ~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:1189:13: warning: comparison of integers of different signs: 'llvm::ArrayRef<long>::value_type' (aka 'long') and 'size_t' (aka 'unsigned long') [-Wsign-compare]
          i >= operandPart.cast<TupleType>().size() || i < 0)
          ~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:1200:13: warning: comparison of integers of different signs: 'llvm::ArrayRef<long>::value_type' (aka 'long') and 'size_t' (aka 'unsigned long') [-Wsign-compare]
          i >= outputPart.cast<TupleType>().size() || i < 0)
          ~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:8899:19: warning: 'hasValue' is deprecated: Use has_value instead [-Wdeprecated-declarations]
  if (parseResult.hasValue()) return parsedType;
                  ^~~~~~~~
                  has_value

Answer 4 · 2022-09-07T00:37:31.000Z

Could mlir-hlo possibly add some CI to prevent these warnings in the first place? -Wsign-compare and -Wdeprecated-declarations seem pretty reasonable to have on.

Answer 5 · 2022-09-07T00:41:14.000Z

-W would only enable warnings, not make them fail the build without -Werror I think.
(We'd have to agree on which compiler we want to have warning free as well)

Answer 6 · 2022-09-07T00:44:54.000Z

I mean, I assume that mlir-hlo developers fix any warnings they see pretty promptly. It doesn't have to be -Werror, but it seems that these warnings regularly go unaddressed (at least that is my anecdotal observation from the Torch-MLIR side).

Answer 7 · 2022-09-07T17:28:11.000Z

"Could mlir-hlo possibly add some CI to prevent these warnings in the first place?". I think that's a great idea.

I am planning to have some spare cycles later this month, once I'm done with integrating StableHLO in MLIR-HLO, to look into MLIR-HLO's CI to make sure that the experience of depending on CHLO/MHLO/StableHLO is great. On the list of things that I'm planning to look into:

Preventing build warning spew.
Preventing CMake build breakages which have been pretty frequent recently.
Providing a minimal target for depending on just MHLO (instead of MHLO + all the MHLO passes + all the GmlSt and THLO dialects and passes).
Considering adding CI for Bazel (at some point recently, our Bazel build was broken, and our CI was fine with that).
Same for CI for Python bindings CMake (same thing for our Python build).

Answer 8 · 2022-09-13T21:43:49.000Z

Here's more that have popped up recently:

ninja: Entering directory `/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/build'
[2786/3177] Building CXX object tools/torch-mlir/mlir-hlo/lib/Dialect/mhlo/IR/CMakeFiles/obj.MhloDialect.dir/hlo_ops.cc.o
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:828:21: warning: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Wsign-compare]
  for (int i = 0; i < getOperands().size(); ++i) {
                  ~ ^ ~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:1180:42: warning: comparison of integers of different signs: 'int64_t' (aka 'long') and 'size_t' (aka 'unsigned long') [-Wsign-compare]
    if (operandIndex < 0 || operandIndex >= operands().size())
                            ~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:1189:13: warning: comparison of integers of different signs: 'llvm::ArrayRef<long>::value_type' (aka 'long') and 'size_t' (aka 'unsigned long') [-Wsign-compare]
          i >= operandPart.cast<TupleType>().size() || i < 0)
          ~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:1200:13: warning: comparison of integers of different signs: 'llvm::ArrayRef<long>::value_type' (aka 'long') and 'size_t' (aka 'unsigned long') [-Wsign-compare]
          i >= outputPart.cast<TupleType>().size() || i < 0)
          ~ ^  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/IR/hlo_ops.cc:8899:19: warning: 'hasValue' is deprecated: Use has_value instead [-Wdeprecated-declarations]
  if (parseResult.hasValue()) return parsedType;
                  ^~~~~~~~
                  has_value
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/llvm-project/llvm/../mlir/include/mlir/IR/OpDefinition.h:48:3: note: 'hasValue' has been explicitly marked deprecated here
  LLVM_DEPRECATED("Use has_value instead", "has_value") bool hasValue() const {
  ^
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/llvm-project/llvm/include/llvm/Support/Compiler.h:143:50: note: expanded from macro 'LLVM_DEPRECATED'
#define LLVM_DEPRECATED(MSG, FIX) __attribute__((deprecated(MSG, FIX)))
                                                 ^
5 warnings generated.
[2787/3177] Building CXX object tools/torch-mlir/mlir-hlo/lib/Dialect/mhlo/transforms/CMakeFiles/obj.ChloPasses.dir/chlo_legalize_to_hlo.cc.o
In file included from /usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/mhlo/transforms/chlo_legalize_to_hlo.cc:28:
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/include/mlir-hlo/Dialect/mhlo/transforms/map_chlo_to_hlo_op.h:52:5: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
    default:
    ^
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/include/mlir-hlo/Dialect/mhlo/transforms/map_chlo_to_hlo_op.h:70:5: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
    default:
    ^
2 warnings generated.
[2788/3177] Building CXX object tools/torch-mlir/mlir-hlo/lib/Dialect/thlo/IR/CMakeFiles/obj.THLODialect.dir/thlo_ops.cc.o
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/lib/Dialect/thlo/IR/thlo_ops.cc:893:31: warning: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'int64_t' (aka 'long') [-Wsign-compare]
  if (reducedInputDims.size() != initType.getRank()) {
      ~~~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~~~~~~
1 warning generated.

Answer 9 · 2022-09-26T11:02:21.000Z

There are a lot more build time warnings related to MHLO and CHLO at the moment.

Answer 10 · 2022-09-26T15:33:01.000Z

Thank you for reporting! @GleasonK will be looking into this both for StableHLO and for MHLO (this ticket).

Answer 11 · 2022-10-17T22:32:30.000Z

Fixed the tablegen spew. Aiming to have any other current build warnings cleaned up this week.

We build StableHLO using -Wall -Werror in our ubuntu clang CI build. Will need to figure out what makes sense for mlir-hlo.

Answer 12 · 2022-11-04T11:31:14.000Z

Saw these today:

/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/include/mlir-hlo/Dialect/mhlo/transforms/map_chlo_to_hlo_op.h:52:5: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
    default:
    ^
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/include/mlir-hlo/Dialect/mhlo/transforms/map_chlo_to_hlo_op.h:70:5: warning: default label in switch which covers all enumeration values [-Wcovered-switch-default]
    default:
    ^

Answer 13 · 2022-11-15T14:53:22.000Z

I'm seeing new warnings:

/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/thlo/IR/thlo_ops.cc:941:29: warning: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'long' [-Wsign-compare]
  if (comparatorArgs.size() != numInputs * 2) {
      ~~~~~~~~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/thlo/IR/thlo_ops.cc:1000:22: warning: comparison of integers of different signs: 'uint64_t' (aka 'unsigned long') and 'int64_t' (aka 'long') [-Wsign-compare]
  if (getDimension() >= referenceRank || getDimension() < 0) {
      ~~~~~~~~~~~~~~ ^  ~~~~~~~~~~~~~
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/thlo/IR/thlo_ops.cc:152:6: warning: unused function 'dimensionsMatch' [-Wunused-function]
bool dimensionsMatch(int64_t d1, int64_t d2) {
     ^
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/thlo/IR/thlo_ops.cc:423:7: warning: unused function 'fuseConcatenateOpThroughPoint' [-Wunused-function]
Value fuseConcatenateOpThroughPoint(ConcatenateOp op, OpBuilder &builder,
      ^
4 warnings generated.

Answer 14 · 2022-12-01T14:52:59.000Z

More warnings:

/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/gml_st/utils/linalg_utils.cc:83:40: warning: comparison of integers of different signs: 'std::tuple_element<0, const llvm::detail::result_pair<llvm::SmallVector<mlir::utils::IteratorType, 12>>>::type' (aka 'const unsigned long') and 'int64_t' (aka 'long') [-Wsign-compare]
    utils::IteratorType expectedTy = i == dim ? utils::IteratorType::reduction
                                     ~ ^  ~~~
1 warning generated.
[2908/3291] Building CXX object tools/torch-mlir/mlir-hlo/thlo/IR/CMakeFiles/obj.THLODialect.dir/thlo_ops.cc.o
/usr/local/google/home/silvasean/pg/torch-mlir/torch-mlir/externals/mlir-hlo/thlo/IR/thlo_ops.cc:890:21: warning: comparison of integers of different signs: 'int' and 'size_t' (aka 'unsigned long') [-Wsign-compare]
  for (int i = 0; i < results.size(); i++) {
                  ~ ^ ~~~~~~~~~~~~~~

-- Can we set up CI for this?

Answer 15 · 2022-12-14T20:08:58.000Z

@ghpvnist will be adding this CI to kokoro. Re-assigning to them.

Answer 16 · 2022-12-17T04:44:07.000Z

Thank you all for keeping this issue updated especially @joker-eph @jpienaar @silvasean and the StableHLO team! The CI has now been added with LLVM_ENABLE_WARNINGS, LLVM_ENABLE_WERROR, and LLVM_ENABLE_PEDANTIC.