Conversation
- histogram.cpp: Main test program with test data - histogram_kernel.cpp: Kernel implementation - histogram_kernel.ll: LLVM IR generated from kernel - histogram_kernel.mlir: MLIR representation from LLVM IR - histogram_kernel_neura.mlir: Neura dialect with compilation issues
There was a problem hiding this comment.
Pull Request Overview
This PR adds testbench files for a histogram computation kernel to the dataflow project. The primary purpose is to provide comprehensive test infrastructure for a histogram kernel implementation across multiple compilation stages.
- Introduces 5 testbench files covering C++ source, LLVM IR, MLIR, and Neura dialect representations
- Highlights compilation issues with LLVM dialect to Neura dialect conversion for
fdivandfptosioperations - Provides a complete test case with input data and expected histogram computation workflow
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| histogram.cpp | Main test program with input data and kernel invocation |
| histogram_kernel.cpp | Simplified kernel implementation for compilation |
| histogram_kernel.ll | LLVM IR representation of the kernel |
| histogram_kernel.mlir | MLIR with LLVM dialect from IR conversion |
| histogram_kernel_neura.mlir | Neura dialect with incomplete conversion showing compilation issues |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| %13 = llvm.fdiv %12, %3 : f32 | ||
| %14 = llvm.fptosi %13 : f32 to i32 |
There was a problem hiding this comment.
These LLVM dialect operations should be converted to Neura dialect operations. The llvm.fdiv should be neura.fdiv and llvm.fptosi should be neura.fptosi to maintain consistency with the rest of the converted code.
| %24 = llvm.fdiv %23, %3 : f32 | ||
| %25 = llvm.fptosi %24 : f32 to i32 |
There was a problem hiding this comment.
These LLVM dialect operations should be converted to Neura dialect operations. The llvm.fdiv should be neura.fdiv and llvm.fptosi should be neura.fptosi to maintain consistency with the rest of the converted code.
There was a problem hiding this comment.
Why upload this? You already upload the entire CGRA-Bench in last PR.
There was a problem hiding this comment.
You only need to have a histogram_test.mlir in this folder and include the command starting with // RUN: , and check whatever you wanna check. WDYT?
There was a problem hiding this comment.
I thought we should have the entire cpp code. So just the kernel's mlir file would be fine?
There was a problem hiding this comment.
You already have that, right?
There was a problem hiding this comment.
You already have that, right?
Yes, but I have some other testbenches that are not included in this repo. Should I upload those c++ files?
There was a problem hiding this comment.
So only mlir file?
There was a problem hiding this comment.
- For other
.ccnot included in theCGRA-Bench, you can create another folder likebenchmarks, and then put them inside it, you can also putCGRA-Benchinside it. So it would look like:- dataflow/test/benchmarks/a.cpp
- dataflow/test/benchmarks/b.cpp
- dataflow/test/benchmarks/CGRA-Bench
- for testing, you can create another folder, e.g.,
e2e, then mimic testhttps://github.com/coredac/dataflow/blob/main/test/neura/for_loop/relu_test.mlir, and provide yourtesting_a.mlirinside thetest/e2e
There was a problem hiding this comment.
Rename your testbench as e2e
I am currently working on compiling the testbench file. Here is one I just uploaded:
We have an issue with this transfer. I attached it to the issues #160
There is a problem that we might need to solve:
%13 = llvm.fdiv %12, %3 : f32
%14 = llvm.fptosi %13 : f32 to i32
There are two ops that are not transferred into the neura operation. I think we might need to add more conversion rules for these Ops.
For fdiv:
I got the error message: 'llvm.fdiv' op operand #0 must be floating point LLVM type or LLVM dialect-compatible vector of floating point LLVM type, but got '!neura.data<f32, i1>'
For fptosi:
We don't have the conversion rules.