[Mlir-commits] [mlir] [mlir][scf]: Avoid using wrong calculation loop pipelining kernel upperbound (PR #116748)

[Aviad Cohen]

================
@@ -444,7 +444,15 @@ scf::ForOp LoopPipelinerInternal::createKernelLoop(
         loc, rewriter.getIntegerAttr(t, maxStage));
     Value maxStageByStep =
         rewriter.create<arith::MulIOp>(loc, step, maxStageValue);
-    newUb = rewriter.create<arith::SubIOp>(loc, ub, maxStageByStep);
+    Value hasAtLeastOneIteration = rewriter.create<arith::CmpIOp>(
+        loc, arith::CmpIPredicate::slt, maxStageByStep, ub);
+    Value possibleNewUB =
+        rewriter.create<arith::SubIOp>(loc, ub, maxStageByStep);
+    // In case of `index` or `unsigned` type, we need to make sure that the
+    // subtraction does not result in a negative value, instead we use lb
+    // to avoid entering the kernel loop.
+    newUb = rewriter.create<arith::SelectOp>(
+        loc, hasAtLeastOneIteration, possibleNewUB, forOp.getLowerBound());
----------------
AviadCo wrote:

`scf.for` definition in the td file requires the `step` to be positive, but doesn't mentioned signed/unsigned. I agree that the pass you mentioned above lowers to signed, but from EmitC lowering I found lowering to `size_t` (which is similar unsigned int):
https://github.com/llvm/llvm-project/blob/abac5be673a2053cceab8ce25009722e45021b9f/mlir/lib/Target/Cpp/TranslateToCpp.cpp#L1667

@jpienaar @marbre @ThomasRaoux Personally I can agree with both signed/unsigned but I would like us to be aligned and define it in the `scf.for` td definition to make it clear. With the current status my platform lowers `scf.for` lb/ub/step into `unsigned int`

https://github.com/llvm/llvm-project/pull/116748