[mlir][linalg] Remove redundant indexOp builder.

[gysit]

Summary

Remove the builder signature taking a signed dimension identifier.

[ergawy]

Thanks I will abandon my review. This fixes the issue for me.

[mehdi_amini]

In general I prefer signed integer when possible, why is this needed?

[gysit]

In D102055#2744787, @mehdi_amini wrote:

In general I prefer signed integer when possible, why is this needed?

There is a default build method taking a uint64_t value that - on some platforms - conflicts with the deleted custom build method taking an int64_t value. In this particular case it seems OK to use an unsigned integer since the dimension is always a positive integer. Otherwise I agree that a signed integer seems to be a better match since the attribute is sign-less.

[mehdi_amini]

In D102055#2744815, @gysit wrote:

In this particular case it seems OK to use an unsigned integer since the dimension is always a positive integer.

That's the reasoning I disagree with in terms of using unsigned actually: I believe we should only use it when a) we want to intentionally wrap around, b) we want to bit-shuffling / bitfields c) we really need this extra bit for large values.
Chandler (and others) explained this in this talk: https://www.youtube.com/watch?v=yG1OZ69H_-o (the part on signed/unsigned arrives around 40 min in the talk, but the entire talk is pretty good).

The only reason we're stuck with unsigned in many places (according to C++ standard committee) is that containers' size() accessor was originally (unfortunately) defined with a size_t.
Bjarne proposed to change it: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2019/p1491r0.pdf

[gysit]

Like the talk!

I had a look and it seems I can replace I64Attr (unsigned) by SI64Attr (signed).

Is there a reason / convention to not use SI64Attr? I didn't see a single instance in the entire code base.

[stellaraccident]

I found this change because one of our bots picked up the state prior and was failing, and upon seeing it, I had the same reaction as Mehdi. Thanks for the build-related fix, but it would also be good to look a bit more at this.

[gysit]

@mehdi_amini @stellaraccident

I looked a bit deeper in the issue and realized why there are no uses of SI64Attr. It does not allow to confine the range of the attribute using the Confined macro:

Arguments<(ins Confined<I64Attr, [IntMinValue<0>]>:$dim)>

The macro ensures we cannot pass a negative (unisigned :)) value to the builder. b.create<IndexOp>(loc, 0-1) fails with the following error:

'linalg.index' op attribute 'dim' failed to satisfy constraint: 64-bit signless integer attribute whose minimum value is 0

There is thus not really a problem with overflows in the negative range. Using an SI64Attr gives us a builder with a signed dim parameter but it does not support the Confined macro meaning we have to write our own verifier. Assuming there is no better option I would probably stick to the existing solution which seems more in line with the rest of the MLIR code base?

[mehdi_amini]

In D102055#2745895, @gysit wrote:

@mehdi_amini @stellaraccident

I looked a bit deeper in the issue and realized why there are no uses of SI64Attr. It does not allow to confine the range of the attribute using the Confined macro:

Is this intended / is there something inherent here or is this just something to implement?

Arguments<(ins Confined<I64Attr, [IntMinValue<0>]>:$dim)>

The macro ensures we cannot pass a negative (unisigned :)) value to the builder. b.create<IndexOp>(loc, 0-1) fails with the following error:

'linalg.index' op attribute 'dim' failed to satisfy constraint: 64-bit signlessf integer attribute whose minimum value is 0
There is thus not really a problem with overflows in the negative range. Using an SI64Attr gives us a builder with a signed dim parameter but it does not support the Confined macro meaning we have to write our own verifier. Assuming there is no better option I would probably stick to the existing solution which seems more in line with the rest of the MLIR code base?

I actually don't quite get what you describe: if a builder is taking an unsigned type, how does the check works to detect a negative input? This seems opposite to what I'd expect...

[mehdi_amini]

(Just to be clear: my comment from yesterday was more about the general "using unsigned for non-negative quantity" explanation you gave than the actual patch here)

[jpienaar]

In D102055#2745895, @gysit wrote:

@mehdi_amini @stellaraccident

I looked a bit deeper in the issue and realized why there are no uses of SI64Attr. It does not allow to confine the range of the attribute using the Confined macro

One can confine the range, one just cannot use IntMinValue as it uses getInt() which can only be used on signless. Something like

class SIntMinValue<int n> : AttrConstraint<
    CPred<"$_self.cast<::mlir::IntegerAttr>().getValue().sge(" # n # ")">,
    "whose minimum value is " # n>;

could be used to confine signed. "intMinValue" means "Signless Int Min Value" (same is true for IntMaxValue).

APInt has isNonNegative, which could be used here to avoid specializing for signless or signed.

[gysit]

(Just to be clear: my comment from yesterday was more about the general "using unsigned for non-negative quantity" explanation you gave than the actual patch here)

Yes I understand that argument 👍 .

APInt has isNonNegative, which could be used here to avoid specializing for signless or signed.

Thanks for the hint. The following seems to work at a first glance:

Arguments<(ins Confined<SI64Attr, [IntNonNegative]>:$dim)>,

I wonder why there are no uses of SI64Attr? Maybe since most dialects work with sign-less integer values? I am fine with both solutions (I64Attr or SI64Attr) and believe the important part is having an overflow check at all.

[gysit]

I actually don't quite get what you describe: if a builder is taking an unsigned type, how does the check works to detect a negative input? This seems opposite to what I'd expect...

The I64Attr is signless and the IntMinValue have signed semantics meaning on the attribute side everything is consistent. ODS then maps a sign-less integer attribute to unsigned in C++...

mlir/include/mlir/Dialect/Linalg/IR/LinalgOps.td

let description = [{
     scf.for %i = %c0 to %0 step %c1 {
       scf.for %j = %c0 to %1 step %c1 {
         store %i, %I[%i, %j] : memref<?x?xindex>
         store %j, %J[%i, %j] : memref<?x?xindex>
       }
     }
     ```
   }];
-  let builders = [
-    OpBuilder<(ins "int64_t":$dim),
-      [{ build($_builder, $_state, $_builder.getIndexType(), dim); }]>
-  ];
 
   let assemblyFormat = [{ $dim attr-dict `:` type($result) }];
 }
 
 #endif // LINALG_OPS

mlir/lib/Dialect/Linalg/Transforms/FusionOnTensors.cpp

generateFusedElementwiseOpRegion(PatternRewriter &rewriter, Operation *fusedOp,
   // 2b. Add an index operation for every fused loop dimension and use the
   // `consumerToProducerLoopsMap` to map the producer indices.
   if (producer.hasIndexSemantics()) {
     // Add an index operation for every fused loop dimension.
     unsigned numFusedOpLoops =
         std::max(producer.getNumLoops(), consumer.getNumLoops());
     SmallVector<Value> fusedIndices;
     fusedIndices.reserve(numFusedOpLoops);
-    llvm::transform(llvm::seq<int64_t>(0, numFusedOpLoops),
-                    std::back_inserter(fusedIndices), [&](int64_t dim) {
+    llvm::transform(llvm::seq<uint64_t>(0, numFusedOpLoops),
+                    std::back_inserter(fusedIndices), [&](uint64_t dim) {
                       return rewriter.create<IndexOp>(producer.getLoc(), dim);
                     });
     for (IndexOp indexOp :
          llvm::make_early_inc_range(producerBlock.getOps<IndexOp>())) {
       Value newIndex = rewriter.create<mlir::AffineApplyOp>(
           producer.getLoc(),
           consumerToProducerLoopsMap.getSubMap(indexOp.dim()), fusedIndices);
       mapper.map(indexOp.getResult(), newIndex);

mlir/lib/Dialect/Linalg/Transforms/Interchange.cpp

assert(op->getNumRegions() == 1 &&
            "expected generic operation to have one block.");
     Block &block = op->getRegion(0).front();
     OpBuilder::InsertionGuard guard(rewriter);
     for (IndexOp indexOp :
          llvm::make_early_inc_range(block.getOps<IndexOp>())) {
       rewriter.setInsertionPoint(indexOp);
       SmallVector<Value> allIndices;
       allIndices.reserve(op.getNumLoops());
-      llvm::transform(llvm::seq<int64_t>(0, op.getNumLoops()),
-                      std::back_inserter(allIndices), [&](int64_t dim) {
+      llvm::transform(llvm::seq<uint64_t>(0, op.getNumLoops()),
+                      std::back_inserter(allIndices), [&](uint64_t dim) {
                         return rewriter.create<IndexOp>(indexOp->getLoc(), dim);
                       });
       rewriter.replaceOpWithNewOp<AffineApplyOp>(
           indexOp, permutationMap.getSubMap(indexOp.dim()), allIndices);
     }
   }
 }