[mlir][spirv] Remove invalid canon pattern for GL.Length

[kuhar]

This rewrite does not preserve numerics: for example, we'd expect the maximum fp value to yield Inf instead of identity.

GL.Length does not allow for fast math flags, so we need to remove this. Special cases (constants) can be handled via a folder if someone wants to implement one.

[llvmbot]

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-spirv

Author: Jakub Kuderski (kuhar)

Changes

This rewrite does not preserve numerics: for example, we'd expect the maximum fp value to yield Inf instead of identity.

GL.Length does not allow for fast math flags, so we need to remove this. Special cases (constants) can be handled via a folder if someone wants to implement one.

---

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

3 Files Affected:

• (modified) mlir/lib/Dialect/SPIRV/IR/SPIRVCanonicalization.td (-8)
• (modified) mlir/lib/Dialect/SPIRV/IR/SPIRVGLCanonicalization.cpp (+2-2)
• (modified) mlir/test/Dialect/SPIRV/Transforms/gl-canonicalize.mlir (-22)

diff --git a/mlir/lib/Dialect/SPIRV/IR/SPIRVCanonicalization.td b/mlir/lib/Dialect/SPIRV/IR/SPIRVCanonicalization.td
index 39fbab8f37a2e..e8d2274d29aa0 100644
--- a/mlir/lib/Dialect/SPIRV/IR/SPIRVCanonicalization.td
+++ b/mlir/lib/Dialect/SPIRV/IR/SPIRVCanonicalization.td
@@ -75,11 +75,3 @@ def ConvertComparisonIntoClamp2_#CmpClampPair[0] : Pat<
         )),
     (CmpClampPair[1] $input, $min, $max)>;
 }
-
-//===----------------------------------------------------------------------===//
-// spirv.GL.Length -> spirv.GL.FAbs
-//===----------------------------------------------------------------------===//
-
-def ConvertGLLengthToGLFAbs : Pat<
-    (SPIRV_GLLengthOp SPIRV_Float:$operand),
-    (SPIRV_GLFAbsOp $operand)>;
diff --git a/mlir/lib/Dialect/SPIRV/IR/SPIRVGLCanonicalization.cpp b/mlir/lib/Dialect/SPIRV/IR/SPIRVGLCanonicalization.cpp
index 46acb8c156fc6..3ad8057a58dc9 100644
--- a/mlir/lib/Dialect/SPIRV/IR/SPIRVGLCanonicalization.cpp
+++ b/mlir/lib/Dialect/SPIRV/IR/SPIRVGLCanonicalization.cpp
@@ -34,8 +34,8 @@ void populateSPIRVGLCanonicalizationPatterns(RewritePatternSet &results) {
               ConvertComparisonIntoClamp2_SPIRV_SLessThanOp,
               ConvertComparisonIntoClamp2_SPIRV_SLessThanEqualOp,
               ConvertComparisonIntoClamp2_SPIRV_ULessThanOp,
-              ConvertComparisonIntoClamp2_SPIRV_ULessThanEqualOp,
-              ConvertGLLengthToGLFAbs>(results.getContext());
+              ConvertComparisonIntoClamp2_SPIRV_ULessThanEqualOp>(
+      results.getContext());
 }
 } // namespace spirv
 } // namespace mlir
diff --git a/mlir/test/Dialect/SPIRV/Transforms/gl-canonicalize.mlir b/mlir/test/Dialect/SPIRV/Transforms/gl-canonicalize.mlir
index 33b877667512e..c1447b38f0a48 100644
--- a/mlir/test/Dialect/SPIRV/Transforms/gl-canonicalize.mlir
+++ b/mlir/test/Dialect/SPIRV/Transforms/gl-canonicalize.mlir
@@ -177,25 +177,3 @@ func.func @clamp_ulessthanequal(%input: i32, %min: i32, %max: i32) -> i32 {
   // CHECK-NEXT: spirv.ReturnValue [[RES]]
   spirv.ReturnValue %2 : i32
 }
-
-// -----
-
-//===----------------------------------------------------------------------===//
-// spirv.GL.Length
-//===----------------------------------------------------------------------===//
-
-// CHECK-LABEL: @convert_length_into_fabs_scalar
-func.func @convert_length_into_fabs_scalar(%arg0 : f32) -> f32 {
-  //CHECK: spirv.GL.FAbs {{%.*}} : f32
-  //CHECK-NOT: spirv.GL.Length
-  %0 = spirv.GL.Length %arg0 : f32 -> f32
-  spirv.ReturnValue %0 : f32
-}
-
-// CHECK-LABEL: @dont_convert_length_into_fabs_vec
-func.func @dont_convert_length_into_fabs_vec(%arg0 : vector<3xf32>) -> f32 {
-  //CHECK: spirv.GL.Length {{%.*}} : vector<3xf32> -> f32
-  //CHECK-NOT: spirv.GL.FAbs
-  %0 = spirv.GL.Length %arg0 : vector<3xf32> -> f32
-  spirv.ReturnValue %0 : f32
-}

[bjacob]

It's a shame though to lose a fast approximation that is perfectly good for many applications, particularly in graphics. Should this motivate a follow-up to support fast-math here (to the extent that LLVM IR is able to meaningfully represent fast-math) ?

[bjacob]

Just realized that this was a canonicalization pattern.

That part was clearly wrong. Fast approximations are great, but they can't be canonicalizations unless they're proven to be exact.

[kuhar]

The spirv spec doesn't support fast math for this op, so it'd be better to do this rewrite at a higher level before we lower to spirv