[mlir][arith][tensor] Disable index type for bitcast #121455

jacquesguan · 2025-01-02T07:31:36Z

Use kInternalStorageBitWidth as the bit width of index type. Fixes #121397.

llvmbot · 2025-01-02T07:32:10Z

@llvm/pr-subscribers-flang-fir-hlfir
@llvm/pr-subscribers-mlir-math
@llvm/pr-subscribers-mlir-ods
@llvm/pr-subscribers-mlir-core
@llvm/pr-subscribers-mlir-tensor

@llvm/pr-subscribers-mlir-arith

Author: Jianjian Guan (jacquesguan)

Changes

Use kInternalStorageBitWidth as the bit width of index type.

Full diff: https://github.com/llvm/llvm-project/pull/121455.diff

2 Files Affected:

(modified) mlir/lib/Dialect/Arith/IR/ArithOps.cpp (+12-1)
(modified) mlir/test/Dialect/Arith/ops.mlir (+6)

diff --git a/mlir/lib/Dialect/Arith/IR/ArithOps.cpp b/mlir/lib/Dialect/Arith/IR/ArithOps.cpp
index d8b314a3fa43c0..6c4aee3aad94fe 100644
--- a/mlir/lib/Dialect/Arith/IR/ArithOps.cpp
+++ b/mlir/lib/Dialect/Arith/IR/ArithOps.cpp
@@ -1723,7 +1723,18 @@ bool arith::BitcastOp::areCastCompatible(TypeRange inputs, TypeRange outputs) {
   if (!srcType || !dstType)
     return false;
 
-  return srcType.getIntOrFloatBitWidth() == dstType.getIntOrFloatBitWidth();
+  unsigned srcWidth, dstWidth;
+  if (auto indexTy = dyn_cast<IndexType>(srcType))
+    srcWidth = IndexType::kInternalStorageBitWidth;
+  else
+    srcWidth = srcType.getIntOrFloatBitWidth();
+
+  if (auto indexTy = dyn_cast<IndexType>(dstType))
+    dstWidth = IndexType::kInternalStorageBitWidth;
+  else
+    dstWidth = dstType.getIntOrFloatBitWidth();
+
+  return srcWidth == dstWidth;
 }
 
 OpFoldResult arith::BitcastOp::fold(FoldAdaptor adaptor) {
diff --git a/mlir/test/Dialect/Arith/ops.mlir b/mlir/test/Dialect/Arith/ops.mlir
index f684e02344a517..46cb1993a3b789 100644
--- a/mlir/test/Dialect/Arith/ops.mlir
+++ b/mlir/test/Dialect/Arith/ops.mlir
@@ -954,6 +954,12 @@ func.func @test_bitcast_scalable_vector1(%arg0 : vector<[8]xf32>) -> vector<[8]x
   return %0 : vector<[8]xi32>
 }
 
+// CHECK-LABEL: test_bitcast_index
+func.func @test_bitcast_index(%arg0 : i64) -> index {
+  %0 = arith.bitcast %arg0 : i64 to index
+  return %0 : index
+}
+
 // CHECK-LABEL: test_cmpi
 func.func @test_cmpi(%arg0 : i64, %arg1 : i64) -> i1 {
   %0 = arith.cmpi ne, %arg0, %arg1 : i64

llvmbot · 2025-01-02T07:32:10Z

@llvm/pr-subscribers-mlir

Author: Jianjian Guan (jacquesguan)

Changes

Use kInternalStorageBitWidth as the bit width of index type.

Full diff: https://github.com/llvm/llvm-project/pull/121455.diff

2 Files Affected:

(modified) mlir/lib/Dialect/Arith/IR/ArithOps.cpp (+12-1)
(modified) mlir/test/Dialect/Arith/ops.mlir (+6)

diff --git a/mlir/lib/Dialect/Arith/IR/ArithOps.cpp b/mlir/lib/Dialect/Arith/IR/ArithOps.cpp
index d8b314a3fa43c0..6c4aee3aad94fe 100644
--- a/mlir/lib/Dialect/Arith/IR/ArithOps.cpp
+++ b/mlir/lib/Dialect/Arith/IR/ArithOps.cpp
@@ -1723,7 +1723,18 @@ bool arith::BitcastOp::areCastCompatible(TypeRange inputs, TypeRange outputs) {
   if (!srcType || !dstType)
     return false;
 
-  return srcType.getIntOrFloatBitWidth() == dstType.getIntOrFloatBitWidth();
+  unsigned srcWidth, dstWidth;
+  if (auto indexTy = dyn_cast<IndexType>(srcType))
+    srcWidth = IndexType::kInternalStorageBitWidth;
+  else
+    srcWidth = srcType.getIntOrFloatBitWidth();
+
+  if (auto indexTy = dyn_cast<IndexType>(dstType))
+    dstWidth = IndexType::kInternalStorageBitWidth;
+  else
+    dstWidth = dstType.getIntOrFloatBitWidth();
+
+  return srcWidth == dstWidth;
 }
 
 OpFoldResult arith::BitcastOp::fold(FoldAdaptor adaptor) {
diff --git a/mlir/test/Dialect/Arith/ops.mlir b/mlir/test/Dialect/Arith/ops.mlir
index f684e02344a517..46cb1993a3b789 100644
--- a/mlir/test/Dialect/Arith/ops.mlir
+++ b/mlir/test/Dialect/Arith/ops.mlir
@@ -954,6 +954,12 @@ func.func @test_bitcast_scalable_vector1(%arg0 : vector<[8]xf32>) -> vector<[8]x
   return %0 : vector<[8]xi32>
 }
 
+// CHECK-LABEL: test_bitcast_index
+func.func @test_bitcast_index(%arg0 : i64) -> index {
+  %0 = arith.bitcast %arg0 : i64 to index
+  return %0 : index
+}
+
 // CHECK-LABEL: test_cmpi
 func.func @test_cmpi(%arg0 : i64, %arg1 : i64) -> i1 {
   %0 = arith.cmpi ne, %arg0, %arg1 : i64

mlir/lib/Dialect/Arith/IR/ArithOps.cpp

CoTinker · 2025-01-02T07:47:19Z

Thanks for your work, and I think we should fix tensor.bitcast, too.

llvm-project/mlir/lib/Dialect/Tensor/IR/TensorOps.cpp

Line 213 in 0c68155

bool BitcastOp::areCastCompatible(TypeRange inputs, TypeRange outputs) {

matthias-springer

I think arith.bitcast should not be used with index type. The bitwidth is unknown, so a bitcast does not make sense. Can we improve the verifier and reject such ops? Note, there is a special index_cast op to convert from/to index type.

Mogball · 2025-01-02T17:07:06Z

I think arith.bitcast should not be used with index type. The bitwidth is unknown, so a bitcast does not make sense. Can we improve the verifier and reject such ops? Note, there is a special index_cast op to convert from/to index type.

+1. A value of index type should be cast to a specific width, such as i32 or i64, before it can be bitcast.

MaheshRavishankar

I dont think the internal storage really represents the actual bitwidth used. That only becomes apparent later on, for example when lowering to LLVM this is set as a field in the data layout used to lower index types.

EDIT: +1 to what was said above as well. Should disallow index types in bitcast operations.

kuhar · 2025-01-02T17:29:51Z

+1. A value of index type should be cast to a specific width, such as i32 or i64, before it can be bitcast.

+1

jacquesguan · 2025-01-04T07:29:32Z

Thanks for all comments, I change this PR to disable index type for arith.bitcast and tensor.bitcast.

mlir/include/mlir/Dialect/Arith/IR/ArithOps.td

CoTinker · 2025-01-04T08:12:48Z

And I think it's better to change the constraint of tensor.bitcast from AnyTensor to TensorOf<[AnySignlessInteger, AnyFloat]>

llvm-project/mlir/include/mlir/Dialect/Tensor/IR/TensorOps.td

Lines 78 to 79 in aa0f334

    
           let arguments = (ins AnyTensor:$source); 
        
           let results = (outs AnyTensor:$dest);

jacquesguan · 2025-01-07T06:20:56Z

And I think it's better to change the constraint of tensor.bitcast from AnyTensor to TensorOf<[AnySignlessInteger, AnyFloat]>

llvm-project/mlir/include/mlir/Dialect/Tensor/IR/TensorOps.td

Lines 78 to 79 in aa0f334

let arguments = (ins AnyTensor:$source);

let results = (outs AnyTensor:$dest);

Done, also add signed and unsigned integer.

matthias-springer · 2025-01-07T08:15:59Z

mlir/include/mlir/IR/CommonTypeConstraints.td

@@ -908,6 +908,11 @@ def BoolLike : TypeOrContainer<I1, "bool-like">;

 def BoolLikeOfAnyRank : TypeOrContainerOfAnyRank<I1, "bool-like">;

+// Type constraint for signless-integer-like types: signless integers, 
+// vectors of signless integers or tensors of signless integers.
+def SignlessInteger : TypeOrValueSemanticsContainer<


This is a bit confusing because there is also SignlessIntegerLike below.

I would change it as follows:

Rename SignlessIntegerLike to SignlessIntegerOrIndexLike.

Rename SignlessIntegerLikeOfAnyRank to SignlessIntegerOrIndexLikeOfAnyRank

Call this new definition SignlessIntegerLike instead of SignlessInteger.

Thanks for comment, I rename these constraints.

mlir/lib/Dialect/Tensor/IR/TensorOps.cpp

CoTinker

The code is LGTM now, and please resolve the tests failure.

Use kInternalStorageBitWidth as the bit width of index type.

jacquesguan requested a review from Mogball January 2, 2025 07:31

llvmbot added mlir mlir:arith labels Jan 2, 2025

jacquesguan mentioned this pull request Jan 2, 2025

[mlir] arith.bitcast crashes on verify when a type is index #121397

Closed

CoTinker reviewed Jan 2, 2025

View reviewed changes

mlir/lib/Dialect/Arith/IR/ArithOps.cpp Outdated Show resolved Hide resolved

CoTinker requested review from kuhar and matthias-springer January 2, 2025 07:48

matthias-springer reviewed Jan 2, 2025

View reviewed changes

MaheshRavishankar requested changes Jan 2, 2025

View reviewed changes

llvmbot added the mlir:tensor label Jan 4, 2025

jacquesguan changed the title ~~[mlir][arith] Support bitcast with index type~~ [mlir][arith][tensor] Disable index type for bitcast Jan 4, 2025

CoTinker reviewed Jan 4, 2025

View reviewed changes

mlir/include/mlir/Dialect/Arith/IR/ArithOps.td Show resolved Hide resolved

llvmbot added mlir:core MLIR Core Infrastructure mlir:ods labels Jan 7, 2025

matthias-springer reviewed Jan 7, 2025

View reviewed changes

llvmbot added the mlir:math label Jan 7, 2025

matthias-springer approved these changes Jan 7, 2025

View reviewed changes

Mogball approved these changes Jan 7, 2025

View reviewed changes

CoTinker reviewed Jan 8, 2025

View reviewed changes

mlir/lib/Dialect/Tensor/IR/TensorOps.cpp Outdated Show resolved Hide resolved

jacquesguan force-pushed the mlir-fix-arith-bitcast branch from 230be88 to 91bf8e2 Compare January 8, 2025 06:25

CoTinker approved these changes Jan 9, 2025

View reviewed changes

[mlir][arith] Support bitcast with index type

a122eee

Use kInternalStorageBitWidth as the bit width of index type.

jacquesguan added 4 commits January 24, 2025 14:49

disable index type for bitcast

4243d03

change type constraint of bitcast

31a5322

Address rename comment

20981e4

Fix flang error

5767f51

jacquesguan force-pushed the mlir-fix-arith-bitcast branch from 91bf8e2 to 5767f51 Compare January 24, 2025 08:11

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Jan 24, 2025

jacquesguan merged commit 990837f into llvm:main Jan 24, 2025
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[mlir][arith][tensor] Disable index type for bitcast #121455

[mlir][arith][tensor] Disable index type for bitcast #121455

Uh oh!

jacquesguan commented Jan 2, 2025 •

edited by CoTinker

Loading

Uh oh!

llvmbot commented Jan 2, 2025 •

edited

Loading

Uh oh!

llvmbot commented Jan 2, 2025

Uh oh!

Uh oh!

CoTinker commented Jan 2, 2025 •

edited

Loading

Uh oh!

matthias-springer left a comment •

edited

Loading

Uh oh!

Mogball commented Jan 2, 2025

Uh oh!

MaheshRavishankar left a comment •

edited

Loading

Uh oh!

kuhar commented Jan 2, 2025

Uh oh!

jacquesguan commented Jan 4, 2025

Uh oh!

Uh oh!

CoTinker commented Jan 4, 2025

Uh oh!

jacquesguan commented Jan 7, 2025

Uh oh!

matthias-springer Jan 7, 2025

Uh oh!

jacquesguan Jan 7, 2025

Uh oh!

Uh oh!

CoTinker left a comment

Uh oh!

Uh oh!

Uh oh!

[mlir][arith][tensor] Disable index type for bitcast #121455

[mlir][arith][tensor] Disable index type for bitcast #121455

Uh oh!

Conversation

jacquesguan commented Jan 2, 2025 • edited by CoTinker Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Jan 2, 2025

Uh oh!

Uh oh!

CoTinker commented Jan 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matthias-springer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mogball commented Jan 2, 2025

Uh oh!

MaheshRavishankar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kuhar commented Jan 2, 2025

Uh oh!

jacquesguan commented Jan 4, 2025

Uh oh!

Uh oh!

CoTinker commented Jan 4, 2025

Uh oh!

jacquesguan commented Jan 7, 2025

Uh oh!

matthias-springer Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

jacquesguan Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

CoTinker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jacquesguan commented Jan 2, 2025 •

edited by CoTinker

Loading

llvmbot commented Jan 2, 2025 •

edited

Loading

CoTinker commented Jan 2, 2025 •

edited

Loading

matthias-springer left a comment •

edited

Loading

MaheshRavishankar left a comment •

edited

Loading