Skip to content

Conversation

@Jerry-Ge
Copy link
Member

@Jerry-Ge Jerry-Ge commented Apr 7, 2025

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Apr 7, 2025

@llvm/pr-subscribers-mlir

@llvm/pr-subscribers-mlir-tosa

Author: Jerry-Ge (Jerry-Ge)

Changes

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

1 Files Affected:

  • (modified) mlir/lib/Dialect/Tosa/IR/TosaOps.cpp (+5)
diff --git a/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp b/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
index c8e9ad8bd3346..80bcc7a618080 100644
--- a/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
+++ b/mlir/lib/Dialect/Tosa/IR/TosaOps.cpp
@@ -600,6 +600,11 @@ LogicalResult tosa::AvgPool2dOp::verify() {
   if (inputETy.isF32() && !accType.isF32())
     return emitOpError("accumulator type for f32 tensor is not f32");
 
+  if ((llvm::isa<Float8E5M2Type>(inputETy) ||
+       llvm::isa<Float8E4M3FNType>(inputETy)) &&
+      !accType.isF16())
+    return emitOpError("accumulator type for f8 tensor is not f16");
+    
   if (inputETy != inputZpETy)
     return emitOpError("expect both input and its zero point are the same "
                        "element type, got ")

@github-actions
Copy link

github-actions bot commented Apr 7, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

Signed-off-by: Jerry Ge <[email protected]>
Change-Id: I04b34f62e622893b52417a96ab33166c96c846d7
Copy link
Contributor

@GeorgeARM GeorgeARM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably add a negative test as well?

if (inputETy.isF32() && !accType.isF32())
return emitOpError("accumulator type for f32 tensor is not f32");

if ((llvm::isa<Float8E5M2Type>(inputETy) ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we move the checks in the validation pass? Maybe we can leave the operator verifier a bit more relaxed ie just check that the accumulator is of fp type?
Thoughts?
Cc: @lhutton1 @FranklandJack

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from me on this - it allows other users of TOSA to express operations that may be important for their workflow e.g. an fp8 avg_pool with an fp32 accumulator

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, thanks for the review. i will then close this PR.

@Jerry-Ge Jerry-Ge closed this Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants