[Mlir-commits] [mlir] [MLIR] Fix an assert that contains a mistake in conditional operator (PR #95668)

[Shivam Gupta]

https://github.com/xgupta created https://github.com/llvm/llvm-project/pull/95668

This is described in (N2) https://pvs-studio.com/en/blog/posts/cpp/1126/ so caught by the PVS Studio analyzer.

Warning message -
V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. LoopEmitter.cpp 983
V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. LoopEmitter.cpp 1039

The assert should be
assert(bArgs.size() == reduc.size() + (needsUniv ? 1 : 0));
since + has higher precedence and ? has lower.

This further can be reduce to
assert(aArgs.size() == reduc.size() + needsUniv);
because needUniv is a bool value which is implicitly converted to 0 or

>From 805b49ab0527322eef3583f477469a84d0cceba0 Mon Sep 17 00:00:00 2001
From: Shivam Gupta <shivam98.tkg at gmail.com>
Date: Sat, 15 Jun 2024 22:23:48 +0530
Subject: [PATCH] [MLIR] Fix an assert that contains a mistake in conditional
 operator

    This is described in https://pvs-studio.com/en/blog/posts/cpp/1126/ so caught by the PVS Studio analyzer.
    Warning message -

    V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. LoopEmitter.cpp 983
    V502 Perhaps the '?:' operator works in a different way than it was expected. The '?:' operator has a lower priority than the '+' operator. LoopEmitter.cpp 1039

    The assert should be
    assert(bArgs.size() == reduc.size() + (needsUniv ? 1 : 0));
    since + has higher precedence and ? has lower.

    This further can be reduce to
    assert(aArgs.size() == reduc.size() + needsUniv);
    becaue needUniv is a bool value which is implicitly conver to 0 or
---
 .../lib/Dialect/SparseTensor/Transforms/Utils/LoopEmitter.cpp | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mlir/lib/Dialect/SparseTensor/Transforms/Utils/LoopEmitter.cpp b/mlir/lib/Dialect/SparseTensor/Transforms/Utils/LoopEmitter.cpp
index 05883f1cefdf3..fe0e515a2d180 100644
--- a/mlir/lib/Dialect/SparseTensor/Transforms/Utils/LoopEmitter.cpp
+++ b/mlir/lib/Dialect/SparseTensor/Transforms/Utils/LoopEmitter.cpp
@@ -542,7 +542,7 @@ std::pair<Operation *, Value> LoopEmitter::emitWhileLoopOverTensorsAtLvls(
   }
   // The remaining block arguments are user-provided reduction values and an
   // optional universal index. Make sure their sizes match.
-  assert(bArgs.size() == reduc.size() + needsUniv ? 1 : 0);
+  assert(bArgs.size() == reduc.size() + needsUniv);
   builder.create<scf::ConditionOp>(loc, whileCond, before->getArguments());
 
   // Generates loop body.
@@ -560,7 +560,7 @@ std::pair<Operation *, Value> LoopEmitter::emitWhileLoopOverTensorsAtLvls(
   }
 
   // In-place update on reduction variable.
-  assert(aArgs.size() == reduc.size() + needsUniv ? 1 : 0);
+  assert(aArgs.size() == reduc.size() + needsUniv);
   for (unsigned i = 0, e = reduc.size(); i < e; i++)
     reduc[i] = aArgs[i];