Index Type to Integer Type conversion

[Nagy H Mostafa]

Hi all, 

I am trying to implement an ArgMin reduction operation which records the minimum element index along a reduction axis. We would like the result to be held in an Integer tensor (as required by our API). I got away with Integer to IndexType conversion by using an identity map and affine.apply. Currently the affine code looks like the following, and the verifier complains about the last store because of the type-mismatch. Any way to do Index to Integer type conversion in MLIR ?

*** Affine Dialect Dump: ***

#map0 = () -> (0)

#map1 = () -> (3)

#map2 = (d0) -> (d0)

#map3 = () -> (4)

func @main(%arg0: memref<4x3xi32>, %arg1: memref<3xi32>, %arg2: index) {

  %c3 = constant 3 : index

  %c0 = constant 0 : index

  %c4 = constant 4 : index

  %c3_0 = constant 3 : index

  %c0_1 = constant 0 : index

  %c0_2 = constant 0 : index

...

  affine.for %i1 = 0 to 4 {

    affine.for %i2 = 0 to 3 {

      %0 = load %arg1[%i2] : memref<3xi32>   // load from result tensor

      %1 = affine.apply #map2(%0)            // convert to index type 

      %2 = load %arg0[%1, %i2] : memref<4x3xi32>  // find new minimum

      %3 = load %arg0[%i1, %i2] : memref<4x3xi32>

      %4 = cmpi "slt", %3, %2 : i32

      %5 = select %4, %i1, %1 : index            // select minimum index

      store %5, %arg1[%i2] : memref<3xi32>   // Store index back to result. Verifier complains here

    }

  }

  return

}

[Alex Zinenko]

Hi Nagy,

there is currently no way of converting `index` to any integer type.  `affine.apply` accepting integers and returning indices actually sounds like a bug to me.

Index has platform-specific bit width as opposed to integers, and can be thought of as intptr_t.  I think the proper way to address the conversion is to provide an explicit cast operation between an index and an integer type.  This should not be hard, but needs a well-specified semantics for the cases where the bit width of the integer does not match that of the index.

As a fast hacky way to get correct IR, declare a function def @convert(%arg : index) -> i32 without providing its body, and call it to do the conversion.  If looking to execute the resulting code on CPU using LLVM, implement a C function int32_t convert(intptr_t arg) {...}, compile it with clang for the relevant target and link the two llvm modules together.

Alex

--
You received this message because you are subscribed to the Google Groups "MLIR" group.
To unsubscribe from this group and stop receiving emails from it, send an email to mlir+uns...@tensorflow.org.
To view this discussion on the web visit https://groups.google.com/a/tensorflow.org/d/msgid/mlir/b0bc7b9c-e0b6-4d3c-b318-cd287409ff40%40tensorflow.org.

--

-- Alex

[Mostafa, Nagy H]

Thanks, Alex.

An explicit cast sounds like the way to go.

 

Thanks for clarifying affine.apply, I was under the impression it can take any SSA Int values as well. Perhaps that can be made more explicit in the spec.

 

We are actually JIT’ing our code, so I will probably work with an IndexType tensor and add a tensor type-conversion call-back at the end.

[Alex Zinenko]

Interestingly, I remember code that disallowed `index` as an element of tensor/vector -- https://github.com/tensorflow/mlir/commit/ca534783b938fc16462d5af297962537ed8afeaf -- with a similar comment about the cast.  Normally, tensors of index types should not be allowed.

--

-- Alex

[Mostafa, Nagy H]

Disallowing IndexType tensor would be better IMO. I am also running into the target-specific IndexType size issue, but luckily I can hack around it since I am compiling for a native target (currently just checking sizeof(intptr_t) :/ )

 

Is Integer <-> Index cast on your roadmap ?

[Chris Lattner]

Agreed, index type shouldn’t be allowed in memref or tensor, and it should be required for affine.apply.

I think we need “affine.cast” sorts of operations, one that takes an integer and returns an index and visa versa.  Any suggestions for a name?  Also, in the case where someone goes from ‘index’ to i12314, should it sign extend or zero extend, or do we need to support both?

-Chris

To view this discussion on the web visit https://groups.google.com/a/tensorflow.org/d/msgid/mlir/CAPL655j%2BD0%3DwDcwsaRC0fKSChQ0r_DrxikHfjj_q6VRMQWBs7w%40mail.gmail.com.

[Uday Kumar Reddy B]

On 6/13/19 7:48 AM, 'Chris Lattner' via MLIR wrote:
> Agreed, index type shouldn’t be allowed in memref or tensor, and it
> should be required for affine.apply.
>
> I think we need “affine.cast” sorts of operations, one that takes an
> integer and returns an index and visa versa.  Any suggestions for a
> name?  Also, in the case where someone goes from ‘index’ to i12314,
> should it sign extend or zero extend, or do we need to support both?

I think we should sign extend, or else variables of index type that
could take negative values (typically loop IVs) would be converted
incorrectly. Parts of memref subscript expressions (also of index type)
could also take on negative values.

~ Uday

>
> -Chris
>
>
>> On Jun 12, 2019, at 3:04 PM, 'Alex Zinenko' via MLIR

>> <ml...@tensorflow.org <mailto:ml...@tensorflow.org>> wrote:
>>
>> Interestingly, I remember code that disallowed `index` as an element
>> of tensor/vector --
>> https://github.com/tensorflow/mlir/commit/ca534783b938fc16462d5af297962537ed8afeaf --
>> with a similar comment about the cast.  Normally, tensors of index
>> types should not be allowed.
>>
>> On Wed, Jun 12, 2019 at 9:49 PM Mostafa, Nagy H

>> <nagy.h....@intel.com <mailto:nagy.h....@intel.com>> wrote:
>>
>> Thanks, Alex. ____
>>
>> An explicit cast sounds like the way to go. ____
>>
>> __ __

>>
>> Thanks for clarifying affine.apply, I was under the impression it
>> can take any SSA Int values as well. Perhaps that can be made more

>> explicit in the spec. ____
>>
>> __ __

>>
>> We are actually JIT’ing our code, so I will probably work with an
>> IndexType tensor and add a tensor type-conversion call-back at the

>> end. ____
>>
>> __ __
>>
>> *From:*Alex Zinenko [mailto:zin...@google.com
>> <mailto:zin...@google.com>]
>> *Sent:* Wednesday, June 12, 2019 12:31 PM
>> *To:* Mostafa, Nagy H <nagy.h....@intel.com
>> <mailto:nagy.h....@intel.com>>
>> *Cc:* MLIR <ml...@tensorflow.org <mailto:ml...@tensorflow.org>>
>> *Subject:* Re: [mlir] Index Type to Integer Type conversion____
>>
>> __ __
>>
>> Hi Nagy,____
>>
>> __ __

>>
>> there is currently no way of converting `index` to any integer
>> type.  `affine.apply` accepting integers and returning indices

>> actually sounds like a bug to me.____
>>
>> __ __

>>
>> Index has platform-specific bit width as opposed to integers, and
>> can be thought of as intptr_t.  I think the proper way to address
>> the conversion is to provide an explicit cast operation between an
>> index and an integer type.  This should not be hard, but needs a
>> well-specified semantics for the cases where the bit width of the

>> integer does not match that of the index.____
>>
>> __ __

>>
>> As a fast hacky way to get correct IR, declare a function
>> def @convert(%arg : index) -> i32 without providing its body, and
>> call it to do the conversion.  If looking to execute the resulting
>> code on CPU using LLVM, implement a C function int32_t
>> convert(intptr_t arg) {...}, compile it with clang for the

>> relevant target and link the two llvm modules together.____
>>
>> __ __
>>
>> Alex____
>>
>> __ __

>>
>> On Wed, Jun 12, 2019 at 9:22 PM Nagy H Mostafa

>> <nagy.h....@intel.com <mailto:nagy.h....@intel.com>>
>> wrote:____
>>
>> Hi all, ____

>>
>> I am trying to implement an ArgMin reduction operation which
>> records the minimum element index along a reduction axis. We
>> would like the result to be held in an Integer tensor (as
>> required by our API). I got away with Integer to IndexType
>> conversion by using an identity map and affine.apply.
>> Currently the affine code looks like the following, and the
>> verifier complains about the last store because of the
>> type-mismatch. Any way to do Index to Integer type conversion

>> in MLIR ?____
>>
>> __ __
>>
>> *** Affine Dialect Dump: ***____
>>
>> #map0 = () -> (0)____
>>
>> #map1 = () -> (3)____
>>
>> #map2 = (d0) -> (d0)____
>>
>> #map3 = () -> (4)____
>>
>> __ __
>>
>> __ __

>>
>> func @main(%arg0: memref<4x3xi32>, %arg1: memref<3xi32>,

>> %arg2: index) {____
>>
>>   %c3 = constant 3 : index____
>>
>>   %c0 = constant 0 : index____
>>
>>   %c4 = constant 4 : index____
>>
>>   %c3_0 = constant 3 : index____
>>
>>   %c0_1 = constant 0 : index____
>>
>>   %c0_2 = constant 0 : index____
>>
>> ...____
>>
>>   affine.for %i1 = 0 to 4 {____
>>
>>     affine.for %i2 = 0 to 3 {____

>>
>>       %0 = load %arg1[%i2] : memref<3xi32>   // load from

>> result tensor____

>>
>>       %1 = affine.apply #map2(%0)            // convert to

>> index type ____

>>
>>       %2 = load %arg0[%1, %i2] : memref<4x3xi32>  // find new

>> minimum____
>>
>>       %3 = load %arg0[%i1, %i2] : memref<4x3xi32>____
>>
>>       %4 = cmpi "slt", %3, %2 : i32____

>>
>>       %5 = select %4, %i1, %1 : index            // select

>> minimum index____

>>
>>       store %5, %arg1[%i2] : memref<3xi32>   // Store index

>> back to result. *Verifier complains here*____
>>
>>     }____
>>
>>   }____
>>
>>   return____
>>
>> }____
>>
>> __ __

>>
>> --
>> You received this message because you are subscribed to the
>> Google Groups "MLIR" group.
>> To unsubscribe from this group and stop receiving emails from
>> it, send an email to mlir+uns...@tensorflow.org

>> <mailto:mlir+uns...@tensorflow.org>.

>> To view this discussion on the web visit
>> https://groups.google.com/a/tensorflow.org/d/msgid/mlir/b0bc7b9c-e0b6-4d3c-b318-cd287409ff40%40tensorflow.org

>> <https://groups.google.com/a/tensorflow.org/d/msgid/mlir/b0bc7b9c-e0b6-4d3c-b318-cd287409ff40%40tensorflow.org?utm_medium=email&utm_source=footer>.____
>>
>>
>> ____
>>
>> __ __
>>
>> -- ____
>>
>> -- Alex____

>>
>>
>>
>> --
>> -- Alex
>>
>> --
>> You received this message because you are subscribed to the Google
>> Groups "MLIR" group.
>> To unsubscribe from this group and stop receiving emails from it, send
>> an email to mlir+uns...@tensorflow.org

>> <mailto:mlir+uns...@tensorflow.org>.

>> To view this discussion on the web visit

>> https://groups.google.com/a/tensorflow.org/d/msgid/mlir/CAPL655j%2BD0%3DwDcwsaRC0fKSChQ0r_DrxikHfjj_q6VRMQWBs7w%40mail.gmail.com
>> <https://groups.google.com/a/tensorflow.org/d/msgid/mlir/CAPL655j%2BD0%3DwDcwsaRC0fKSChQ0r_DrxikHfjj_q6VRMQWBs7w%40mail.gmail.com?utm_medium=email&utm_source=footer>.

>
> --
> You received this message because you are subscribed to the Google
> Groups "MLIR" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to mlir+uns...@tensorflow.org

> <mailto:mlir+uns...@tensorflow.org>.

> To view this discussion on the web visit

> https://groups.google.com/a/tensorflow.org/d/msgid/mlir/3F974E44-CBFF-41B8-AC97-8DA9A5CD1DEF%40google.com
> <https://groups.google.com/a/tensorflow.org/d/msgid/mlir/3F974E44-CBFF-41B8-AC97-8DA9A5CD1DEF%40google.com?utm_medium=email&utm_source=footer>.

[Alex Zinenko]

What about the inverse case, converting index(312) to i8?  Do we make this UB or do always truncate?

--

-- Alex

[Chris Lattner]

I’d recommend always truncating on this.

-Chris

[Alex Zinenko]

That works for me.  I will be adding an index cast operation and its lowering to LLVM IR and banning affine.apply on non-index types.

--

-- Alex

[Chris Lattner]

Awesome, thanks Alex!

-Chris

[Alex Zinenko]

FYI, we added checks to affine.apply so that it only takes and produces values of index type, and provided an index_cast operation to cast between index and integer scalars (vectors/tensors of index are disallowed anyway) as of https://github.com/tensorflow/mlir/commit/8b5b9b90cae4755ef18b4edc407cc6462156a1ad

--

-- Alex

[Mostafa, Nagy H]

Good news! We will give it a try.